Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@pengowray
Copy link

Fixes: #33577 TimeSpan.Parse gives inconsistent and incorrect results with 8 fractional digits.

InvalidFractionTest() is the culprit. It fails all the same things I was failing before. Adjusted code passes all the tests I can throw at it. Simply an off-by-one error (which becomes an off-by-ten error thanks to logarithms).

TimeSpan.Parse has some fairly convoluted logic when it gets to IsInvalidFraction(). It attempts to reconstruct the number of fractional digits in a TTT token based on the number of leading zeroes and the integer number itself, or rather, it avoids reconstructing it, and just attempts to check if there's not too many digits, involving the largest possible number, divided by 10 or 100 etc depending on the number of leading zeroes. It was getting this wrong by 10 or by one digit, which meant it was falling back onto some weaker checks earlier in the code. These gave very odd results (e.g. different results for numbers ending in 99 and 98)

This patch fixes that very simply.

I've created an InvalidFractionTest in my own repo to test the code against all the cases I could think of, but I haven't attempted to integrate the Unit Tests into corefx's codebase. This patch just has the fix itself. So I assume you'll want some tests before committing.

@danmoseley
Copy link
Member

Thanks @Quole . Yes, please let's include tests as part of the commit.

@jkotas
Copy link
Member

jkotas commented Nov 19, 2018

This is change in CoreLib. It should be submitted via CoreCLR repo. This PR can be re-purposed to have test only.

@jkotas
Copy link
Member

jkotas commented Nov 19, 2018

The details are explained in https://github.com/dotnet/coreclr/blob/master/Documentation/project-docs/adding_new_public_apis.md. The doc talks about adding new APIs, but it applies for bug fixes in the existing APIs as well.

@karelz karelz added the blocked Issue/PR is blocked on something - see comments label Dec 19, 2018
@karelz karelz changed the title bugfix for #33577: 8+ fractional digit checking (TimeSpan.Parse) Fix 8+ fractional digit checking (TimeSpan.Parse) Dec 20, 2018
@karelz karelz added this to the 3.0 milestone Dec 21, 2018
@stephentoub
Copy link
Member

dotnet/coreclr#21077 is for the product change, and #33925 is addressing tests, so closing this one as a dup.

@stephentoub stephentoub closed this Jan 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Globalization blocked Issue/PR is blocked on something - see comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TimeSpan.Parse gives inconsistent and incorrect results with 8 fractional digits

7 participants