Skip to content
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 parsing of trailing zeros: TryParseNumber #47666

Merged
merged 14 commits into from
Feb 18, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ public static void WriteInteger_SingleValue_HappyPath(string valueString, string
[InlineData("1", "c4820001")]
[InlineData("-1", "c4820020")]
[InlineData("1.1", "c482200b")]
[InlineData("1.000", "c482221903e8")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with cbor. Do we know why this changed in size?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nor am I. I just figured that we are parsing correctly now and returning 1. So I looked at the test case 2 lines above for 1 and copied that. I can go back and see what value we're returning without the bug fix, but I'm pretty sure it is wrong somehow and this PR is fixing it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to get confirmation.

The number of hex digits implies it was over the length 32-bits, but it also looks like there is some general encoding (compression?) here so it's unclear whether the change is actually correct (I would guess it is though).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eiriktsarpalis for thoughts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original CBOR encodes 1000e-3, while the new one is 1e0, so I presume this is in line with the intended change in the parser.

[InlineData("1.000", "c4820001")]
[InlineData("273.15", "c48221196ab3")]
[InlineData("79228162514264337593543950335", "c48200c24cffffffffffffffffffffffff")] // decimal.MaxValue
[InlineData("7922816251426433759354395033.5", "c48220c24cffffffffffffffffffffffff")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,28 @@ public void CheckConsistency()

Debug.Assert(numDigits == DigitsCount, "Null terminator found in unexpected location in Number");
Debug.Assert(numDigits < Digits.Length, "Null terminator not found in Number");

uint totalDigits = (uint)(DigitsCount);
uint positiveExponent = (uint)(Math.Max(0, Scale));
uint integerDigitsPresent = Math.Min(positiveExponent, totalDigits);
uint fractionalDigitsPresent = totalDigits - integerDigitsPresent;

// Verify that the fractional part does not have any trailing zeros
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
int numberOfTrailingZeros = 0;
for (int i = DigitsCount - (int)fractionalDigitsPresent; i < DigitsCount; i++)
pgovind marked this conversation as resolved.
Show resolved Hide resolved
{
byte digit = Digits[i];
if (digit == 0)
pgovind marked this conversation as resolved.
Show resolved Hide resolved
{
numberOfTrailingZeros++;
}
else
{
numberOfTrailingZeros = 0;
}
}
Debug.Assert(numberOfTrailingZeros == 0, "Fractional part should not have trailing zeros");

#endif // DEBUG
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@ private static unsafe bool TryParseNumber(ref char* str, char* strEnd, NumberSty
int digCount = 0;
int digEnd = 0;
int maxDigCount = number.Digits.Length - 1;
int numberOfTrailingZeros = 0;

while (true)
{
Expand Down Expand Up @@ -357,6 +358,18 @@ private static unsafe bool TryParseNumber(ref char* str, char* strEnd, NumberSty
{
number.Scale++;
}
else if (digCount < maxDigCount)
{
// Handle a case like "53.0". We need to ignore trailing zeros in the fractional part, so we keep a count of the number of trailing zeros and update digCount later
if (ch == '0')
{
numberOfTrailingZeros++;
}
else
{
numberOfTrailingZeros = 0;
}
}
state |= StateNonZero;
}
else if ((state & StateDecimal) != 0)
Expand All @@ -381,8 +394,15 @@ private static unsafe bool TryParseNumber(ref char* str, char* strEnd, NumberSty
}

bool negExp = false;
number.DigitsCount = digEnd;
number.DigitsCount = digEnd - numberOfTrailingZeros;
number.Digits[digEnd] = (byte)('\0');
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
if (numberOfTrailingZeros != 0)
{
for (int i = digEnd - numberOfTrailingZeros; i < digEnd; i++)
{
number.Digits[i] = (byte)('\0');
}
}
if ((state & StateDigits) != 0)
{
if ((ch == 'E' || ch == 'e') && ((styles & NumberStyles.AllowExponent) != 0))
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Runtime/tests/System/DecimalTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,7 @@ public static void Negate(decimal d, decimal expected)
public static IEnumerable<object[]> Parse_Valid_TestData()
{
NumberStyles defaultStyle = NumberStyles.Number;
NumberFormatInfo invariantFormat = NumberFormatInfo.InvariantInfo;

NumberFormatInfo emptyFormat = NumberFormatInfo.CurrentInfo;

Expand All @@ -788,6 +789,7 @@ public static IEnumerable<object[]> Parse_Valid_TestData()
yield return new object[] { " 123 ", defaultStyle, null, 123m };
yield return new object[] { (567.89m).ToString(), defaultStyle, null, 567.89m };
yield return new object[] { (-567.89m).ToString(), defaultStyle, null, -567.89m };
yield return new object[] { "0.6666666666666666666666666666500000000000000000000000000000000000000000000000000000000000000", defaultStyle, invariantFormat, 0.6666666666666666666666666666m };

yield return new object[] { "79228162514264337593543950335", defaultStyle, null, 79228162514264337593543950335m };
yield return new object[] { "-79228162514264337593543950335", defaultStyle, null, -79228162514264337593543950335m };
Expand Down
7 changes: 7 additions & 0 deletions src/libraries/System.Runtime/tests/System/DoubleTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,13 @@ public static IEnumerable<object[]> Parse_Valid_TestData()
yield return new object[] { (567.89).ToString(), defaultStyle, null, 567.89 };
yield return new object[] { (-567.89).ToString(), defaultStyle, null, -567.89 };
yield return new object[] { "1E23", defaultStyle, null, 1E23 };
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
yield return new object[] { "9007199254740997.0", defaultStyle, invariantFormat, 9007199254740996.0 };
yield return new object[] { "5.005", defaultStyle, invariantFormat, 5.005 };
yield return new object[] { "5.050", defaultStyle, invariantFormat, 5.05 };
yield return new object[] { "5005.0", defaultStyle, invariantFormat, 5005.0 };
yield return new object[] { "50050.0", defaultStyle, invariantFormat, 50050.0 };
yield return new object[] { "5005", defaultStyle, invariantFormat, 5005.0 };
yield return new object[] { "50050", defaultStyle, invariantFormat, 50050.0 };

tannergooding marked this conversation as resolved.
Show resolved Hide resolved
yield return new object[] { (123.1).ToString(), NumberStyles.AllowDecimalPoint, null, 123.1 };
yield return new object[] { (1000.0).ToString("N0"), NumberStyles.AllowThousands, null, 1000.0 };
Expand Down