Fix BigInteger.Parse returning incorrect values for exponents above 1000 #73643
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
| public static IEnumerable<object[]> RunFormatScientificNotationToBigIntegerAndViceVersaData() | ||
| { | ||
| yield return new object[] { "1E+1000", "1E+1000" }; | ||
| yield return new object[] { "1E+1001", "1E+1001" }; |
There was a problem hiding this comment.
@tannergooding I was not able to get a test with "1E+2147483639" to work locally, I think it ran for too long.
There was a problem hiding this comment.
I think that's fine, as long as we have some basic tests that values over 1E+1000 work it should be good enough
Testing the extreme edge cases is going to run into lots of issues.
There was a problem hiding this comment.
I added a couple bigger test cases.
| } | ||
| } | ||
| } while (char.IsAsciiDigit(ch)); | ||
| } while (char.IsAsciiDigit(ch) && (exp < int.MaxValue / 10)); |
There was a problem hiding this comment.
Would it be better to limit this to 999,999,999 like we did for ToString("G#########")?
There was a problem hiding this comment.
It might be safer in case there is any incrementing that happens elsewhere that I hadn't noticed. Would that require updating documentation somewhere?
There was a problem hiding this comment.
Likely a similar breaking change doc to what we did for ToString.
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // Set exp to Int.MaxValue to signify the requested exponent is too large. This will lead to an OverflowException later. | ||
| exp = int.MaxValue; | ||
| number.scale = 0; |
There was a problem hiding this comment.
@tannergooding negative exponents already had another in-use code path, so we have to use int.MaxValue as our Overflow signifier instead of -1. Additonally, number.scale has to be set to 0 because sometimes it's non-zero at this point and the addition on line 533 will overflow.
| @@ -34,7 +34,7 @@ public static IEnumerable<object[]> Cultures | |||
| [Theory] | |||
| [MemberData(nameof(Cultures))] | |||
| [OuterLoop] | |||
| public static void RunParseToStringTests(CultureInfo culture) | |||
| public static void RunParseToStringTests(CultureInfo culture) | |||
There was a problem hiding this comment.
| public static void RunParseToStringTests(CultureInfo culture) | |
| public static void RunParseToStringTests(CultureInfo culture) |
|
Improvements on dotnet/perf-autofiling-issues#7327 |
ghost
locked as resolved and limited conversation to collaborators
Fixes #17296
Wrapping up the effort that was started here: #55397, I added the review suggestions the original author did not get to.