-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix BigInteger.Parse returning incorrect values for exponents above 1000 #73643
Changes from 3 commits
3de6672
614fbce
318fa97
9401318
f5a15da
4264827
16c134b
0221eec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -439,6 +439,38 @@ public static void CustomFormatPerMille() | |
RunCustomFormatToStringTests(s_random, "#\u2030000000", CultureInfo.CurrentCulture.NumberFormat.NegativeSign, 6, PerMilleSymbolFormatter); | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a couple bigger test cases. |
||
} | ||
|
||
[Theory] | ||
[MemberData(nameof(RunFormatScientificNotationToBigIntegerAndViceVersaData))] | ||
public static void RunFormatScientificNotationToBigIntegerAndViceVersa(string testingValue, string expectedResult) | ||
{ | ||
BigInteger parsedValue; | ||
string actualResult; | ||
|
||
parsedValue = BigInteger.Parse(testingValue, NumberStyles.AllowExponent); | ||
actualResult = parsedValue.ToString("E0"); | ||
|
||
Assert.Equal(expectedResult, actualResult); | ||
} | ||
|
||
public static IEnumerable<object[]> RunFormatScientificNotationToBigIntegerThrowsExceptionData() | ||
{ | ||
yield return new object[] { "1E+2147483647" }; | ||
yield return new object[] { "1E+21474836492" }; | ||
} | ||
|
||
[Theory] | ||
[MemberData(nameof(RunFormatScientificNotationToBigIntegerThrowsExceptionData))] | ||
public static void RunFormatScientificNotationToBigIntegerThrowsException(string testingValue) | ||
{ | ||
Assert.Throws<PlatformNotSupportedException>(() => BigInteger.Parse(testingValue, NumberStyles.AllowExponent)); | ||
} | ||
|
||
private static void RunSimpleProviderToStringTests(Random random, string format, NumberFormatInfo provider, int precision, StringFormatter formatter) | ||
{ | ||
string test; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to limit this to
999,999,999
like we did forToString("G#########")
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely a similar breaking change doc to what we did for
ToString
.