Fixing the double/float formatting code to use a fallback precision for custom-format strings. #22522
Conversation
…or custom-format strings.
|
CC. @danmosemsft, @jkotas, @stephentoub |
|
|
||
| // 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). |
There was a problem hiding this comment.
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.
|
Is there going to be a matching corefx test update/addition? |
|
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. |
Yes. I plan on adding some tests to cover this code-path. |
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 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 |
|
Also CC. @eerhardt who was reviewed many of the previous changes here. |
| char fmt = ParseFormatSpecifier(format, out int precision); | ||
| byte* pDigits = stackalloc byte[DoubleNumberBufferLength]; | ||
|
|
||
| if (fmt == '\0') |
There was a problem hiding this comment.
(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.
There was a problem hiding this comment.
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.
|
Any other feedback here, or is this good to merge? |
…or custom-format strings. (dotnet/coreclr#22522) Commit migrated from dotnet/coreclr@556e22d
3 participants
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.