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

Fix up the majority of IEEE compliance bugs for Double/Single ToString and Parse #19905

Closed
wants to merge 7 commits into from
Closed

Fix up the majority of IEEE compliance bugs for Double/Single ToString and Parse #19905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0785a5a
Updating Number.Parsing to properly recover -0
tannergooding Sep 11, 2018
6ff3240
Updating Double/Single to ignore case when parsing NaN, PositiveInfin…
tannergooding Sep 11, 2018
351abbb
Updating Single/Double to always roundtrip to the correct number of d…
tannergooding Sep 11, 2018
786929e
Updating Single/Double to keep the "prettier" default digit count for…
tannergooding Sep 12, 2018
a909896
Updating DoubleToNumber to support up to 99 digits, which is the max …
tannergooding Sep 12, 2018
db591d5
Fixing FormatDouble and FormatSingle to preserve the requested number…
tannergooding Sep 12, 2018
571e2ab
Disabling various CoreFX tests that were dependent on incorrect round…
tannergooding Sep 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/System.Private.CoreLib/shared/System/Double.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,15 +345,15 @@ private static bool TryParse(ReadOnlySpan<char> s, NumberStyles style, NumberFor
if (!success)
{
ReadOnlySpan<char> sTrim = s.Trim();
if (sTrim.EqualsOrdinal(info.PositiveInfinitySymbol))
if (sTrim.EqualsOrdinalIgnoreCase(info.PositiveInfinitySymbol))
{
result = PositiveInfinity;
}
else if (sTrim.EqualsOrdinal(info.NegativeInfinitySymbol))
else if (sTrim.EqualsOrdinalIgnoreCase(info.NegativeInfinitySymbol))
{
result = NegativeInfinity;
}
else if (sTrim.EqualsOrdinal(info.NaNSymbol))
else if (sTrim.EqualsOrdinalIgnoreCase(info.NaNSymbol))
{
result = NaN;
}
Expand Down
149 changes: 54 additions & 95 deletions src/System.Private.CoreLib/shared/System/Number.Formatting.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,19 @@ namespace System
internal static partial class Number
{
internal const int DecimalPrecision = 29; // Decimal.DecCalc also uses this value
private const int FloatPrecision = 7;
private const int DoublePrecision = 15;
tannergooding marked this conversation as resolved.
Show resolved Hide resolved

// IEEE Requires at least 9 digits for roundtripping. However, using a lower number
Copy link
Member

Choose a reason for hiding this comment

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

Which paragraph does require this? I am not able to find it.

Copy link
Member Author

Choose a reason for hiding this comment

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

"5.12.2 External decimal character sequences representing finite numbers"

― for binary32, Pmin (binary32) = 9
― for binary64, Pmin (binary64) = 17

Conversions from a supported binary format bf to an external character sequence and back again
results in a copy of the original number so long as there are at least Pmin (bf ) significant digits
specified and the rounding-direction attributes in effect during the two conversions are round to
nearest rounding-direction attributes.

Copy link
Member

Choose a reason for hiding this comment

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

I do not read this as requirement. This paragraph just explains consequences of what is written above.

This says that round-tripping can be achieved by always having at least Pmin(bf) significant digits. It does not say that this is the only way to achieve round-tripping.

// of default digits for the other specifiers results in "prettier" numbers and more
// backwards compatible behavior.
private const int DefaultSingleDigits = 7;
private const int DefaultSinglePrecision = 9;

// IEEE Requires at least 17 digits for roundtripping. However, using a lower number
// of default digits for the other specifiers results in "prettier" numbers and more
// backwards compatible behavior.
private const int DefaultDoubleDigits = 15;
private const int DefaultDoublePrecision = 17;

private const int ScaleNAN = unchecked((int)0x80000000);
private const int ScaleINF = 0x7FFFFFFF;
private const int MaxUInt32DecDigits = 10;
Expand Down Expand Up @@ -387,61 +398,34 @@ public static bool TryFormatDouble(double value, ReadOnlySpan<char> format, Numb
private static string FormatDouble(ref ValueStringBuilder sb, double value, ReadOnlySpan<char> format, NumberFormatInfo info)
{
char fmt = ParseFormatSpecifier(format, out int digits);
int precision = DoublePrecision;

NumberBuffer number = default;
number.kind = NumberBufferKind.Double;

switch (fmt)
{
case 'R':
case 'r':
{
// In order to give numbers that are both friendly to display and round-trippable, we parse the
// number using 15 digits and then determine if it round trips to the same value. If it does, we
// convert that NUMBER to a string, otherwise we reparse using 17 digits and display that.
DoubleToNumber(value, DoublePrecision, ref number);
if (number.scale == ScaleNAN)
{
return info.NaNSymbol;
}
else if (number.scale == ScaleINF)
{
return number.sign ? info.NegativeInfinitySymbol : info.PositiveInfinitySymbol;
}

if (NumberToDouble(ref number) == value)
{
NumberToString(ref sb, ref number, 'G', DoublePrecision, info);
}
else
{
DoubleToNumber(value, 17, ref number);
NumberToString(ref sb, ref number, 'G', 17, info);
}
int precision;

return null;
}

case 'E':
case 'e':
// Round values less than E14 to 15 digits
if (digits > 14)
{
precision = 17;
}
break;
if ((fmt == 'R') || (fmt == 'r'))
{
fmt = 'G';
precision = DefaultDoublePrecision;

case 'G':
case 'g':
// Round values less than G15 to 15 digits. G16 and G17 will not be touched.
if (digits > 15)
{
precision = 17;
}
break;
// IEEE Roundtripping requires us to include at least `17` digits
digits = precision;
}
else if (digits <= 0)
{
// This ensures that, for the default case, we return a string containing the old
// number of digits (15), which results in "prettier" numbers for most cases.
precision = DefaultDoubleDigits;
}
else
{
// IEEE requires we correctly round to the requested number of digits
precision = digits;
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to update this logic so that we preserve the fact that the user didn't explicitly specify a digit count, or that the user specified 0 for the digit count.

This makes a difference for things like double.ToString("N") which i had accidentally made to be treated like double.ToString("N15").

Copy link
Member Author

Choose a reason for hiding this comment

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

Now, digits is always preserved (except for "R", where it is explicitly documented to be ignored) and precision is updated accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

This brought the CoreFX failure count down to 354, and they are now all XML and JSON serialization differences plus the UTF8Parser differences (which need fixup and for which the code lives in CoreFX).

}

DoubleToNumber(value, precision, ref number);

if (number.scale == ScaleNAN)
{
return info.NaNSymbol;
Expand Down Expand Up @@ -488,60 +472,34 @@ public static bool TryFormatSingle(float value, ReadOnlySpan<char> format, Numbe
private static string FormatSingle(ref ValueStringBuilder sb, float value, ReadOnlySpan<char> format, NumberFormatInfo info)
{
char fmt = ParseFormatSpecifier(format, out int digits);
int precision = FloatPrecision;

NumberBuffer number = default;
number.kind = NumberBufferKind.Double;

switch (fmt)
{
case 'R':
case 'r':
{
// In order to give numbers that are both friendly to display and round-trippable, we parse the
// number using 7 digits and then determine if it round trips to the same value. If it does, we
// convert that NUMBER to a string, otherwise we reparse using 9 digits and display that.
DoubleToNumber(value, FloatPrecision, ref number);
if (number.scale == ScaleNAN)
{
return info.NaNSymbol;
}
else if (number.scale == ScaleINF)
{
return number.sign ? info.NegativeInfinitySymbol : info.PositiveInfinitySymbol;
}
int precision;

if ((float)NumberToDouble(ref number) == value)
{
NumberToString(ref sb, ref number, 'G', FloatPrecision, info);
}
else
{
DoubleToNumber(value, 9, ref number);
NumberToString(ref sb, ref number, 'G', 9, info);
}
return null;
}

case 'E':
case 'e':
// Round values less than E14 to 15 digits.
if (digits > 6)
{
precision = 9;
}
break;
if ((fmt == 'R') || (fmt == 'r'))
{
fmt = 'G';
precision = DefaultSinglePrecision;

case 'G':
case 'g':
// Round values less than G15 to 15 digits. G16 and G17 will not be touched.
if (digits > 7)
{
precision = 9;
}
break;
// IEEE Roundtripping requires us to include at least `9` digits
digits = digits;
}
else if (digits <= 0)
{
// This ensures that, for the default case, we return a string containing the old
// number of digits (7), which results in "prettier" numbers for most cases.
precision = DefaultSingleDigits;
}
else
{
// IEEE requires we correctly round to the requested number of digits
precision = digits;
}

DoubleToNumber(value, precision, ref number);

if (number.scale == ScaleNAN)
{
return info.NaNSymbol;
Expand All @@ -559,6 +517,7 @@ private static string FormatSingle(ref ValueStringBuilder sb, float value, ReadO
{
NumberToStringFormat(ref sb, ref number, format, info);
}

return null;
}

Expand Down
4 changes: 3 additions & 1 deletion src/System.Private.CoreLib/shared/System/Number.NumberBuffer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ namespace System
{
internal static partial class Number
{
private const int NumberMaxDigits = 50; // needs to == NUMBER_MAXDIGITS in coreclr's src/classlibnative/bcltype/number.h.
// needs to == NUMBER_MAXDIGITS in coreclr's src/classlibnative/bcltype/number.h.
// We currently set this to 99, as that is the maximum precision allowed for the standard format strings
private const int NumberMaxDigits = 99;

[StructLayout(LayoutKind.Sequential, Pack = 1)]
internal unsafe ref struct NumberBuffer // needs to match layout of NUMBER in coreclr's src/classlibnative/bcltype/number.h
Expand Down
Loading