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

Fixing the double/float formatting code to use a fallback precision for custom-format strings. #22522

Merged
merged 1 commit into from Feb 13, 2019
Merged

Conversation

@tannergooding
Copy link
Member

@tannergooding tannergooding commented Feb 11, 2019

This ensures that custom-format strings that are requesting between 1-15 digits continue printing the same as before (as was the case with the standard-format strings). For custom-format strings in particular, and unlike standard-format strings, they will also continue printing the same number of digits as before when the custom-format string is requesting above 15 digits. I do think it would be desirable to "pre-parse" the custom-format string and determine exactly how many digits the user is requesting (and I believe we could do so in a way that does not cause a perf-regression and that may actually provide some perf-gains for common cases), but that is a more-involved change.

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Feb 11, 2019

CC. @danmosemsft, @jkotas, @stephentoub

Loading

@@ -250,6 +250,15 @@ internal static partial class Number
private const int SinglePrecision = 9;
private const int DoublePrecision = 17;

// SinglePrecisionCustomFormat and DoublePrecisionCustomFormat are used to ensure that
// custom format strings return the same string as in previous releases when the format
// would return x digits or less (where x is the value of the corresponding constant).
Copy link
Member Author

@tannergooding tannergooding Feb 11, 2019

Choose a reason for hiding this comment

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

This part in particular (returning the same string as before when less than x digits are requested) is also true for standard-numeric format strings. For the standard-formats, we only differ when more than 15 digits are requested and for the default case on G and R.

Loading

@jkotas
Copy link
Member

@jkotas jkotas commented Feb 11, 2019

Is there going to be a matching corefx test update/addition?

Loading

@jkotas
Copy link
Member

@jkotas jkotas commented Feb 11, 2019

I would be ok with me to wait until somebody reports it as a real problem; but if you would like to do it now - that is fine with me as well.

Loading

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Feb 11, 2019

Is there going to be a matching corefx test update/addition?

Yes. I plan on adding some tests to cover this code-path.

Loading

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Feb 11, 2019

I would be ok with me to wait until somebody reports it as a real problem; but if you would like to do it now - that is fine with me as well.

For this particular case, I feel it is better to cover it now. Not specifying an explicit precision means that the digit generator will produce the shortest roundtrippable string, rather than the first 'X' significant digits. This results in cases where the number differs and it is not clearly better. As an extreme example, you have values like double.Epsilon where the digit generator will produce 5 rather than 494065645841247. Then, when formatting if the user has requested 2 significant digits, they will get just 5E-324 rather than 4.9E-324.

For people who are relying on custom-format strings (who are likely users expecting a particular format) to return the requested number of digits, this is a significant breaking change (especially for common cases like "0.00" or "0.0E0"). I have, in general, tried to ensure that users explicitly requesting a given number of digits are getting exactly what they requested (and for requests less than 15 digits, we can make this completely non-breaking).

Loading

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Feb 12, 2019

Also CC. @eerhardt who was reviewed many of the previous changes here.

Loading

@@ -525,6 +534,13 @@ private static unsafe string FormatDouble(ref ValueStringBuilder sb, double valu
char fmt = ParseFormatSpecifier(format, out int precision);
byte* pDigits = stackalloc byte[DoubleNumberBufferLength];

if (fmt == '\0')
Copy link
Member

@eerhardt eerhardt Feb 12, 2019

Choose a reason for hiding this comment

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

(super nit): I think we should be consistent with how we are checking the fmt variable in this method.

Here we are using fmt == '\0'. Below: fmt != 0.

Loading

Copy link
Member Author

@tannergooding tannergooding Feb 12, 2019

Choose a reason for hiding this comment

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

I can update the below one in a separate PR.

I think explicitly checking '\0' makes it much clearer that we are checking for the terminating null.

Loading

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Feb 12, 2019

Any other feedback here, or is this good to merge?

Loading

@tannergooding tannergooding merged commit 556e22d into dotnet:master Feb 13, 2019
67 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants