fix confusion about parts required for float formatting #41859
Conversation
The documentation for flt2dec doesn't match up with the actual implementation, so fix the documentation to align with reality. Presumably due to the mismatch, the formatting code for floats in std::fmt can use correspondingly shorter arrays in some places, so fix those places up as well. Fixes rust-lang#41304.
|
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
| /// There should be at least 5 parts available, due to the worst case like | ||
| /// `[+][0.][0000][45][0000]` with `frac_digits = 10`. | ||
| /// There should be at least 4 parts available, due to the worst case like | ||
| /// `[+][0.][0000][2][0000]` with `frac_digits = 10`. |
There was a problem hiding this comment.
??? I still count 5 parts +, 0., 0000, 2 and 0000.
There was a problem hiding this comment.
It's unfortunate that the [] notation is being used in two ways here: to denote a Part and to--I think--indicate the optional quality of the sign of the formatted number. Since the [+] is being used in the latter sense, it does not count as a Part that would be stored in the parts array.
There was a problem hiding this comment.
OK, perhaps the + should use a different bracket or no brackets, or just omit the + altogether. Also, why the 45 is changed to 2? That doesn't seem to make any difference.
There was a problem hiding this comment.
My understanding of the use of the numbers here was that they were denoting the index of the associated Part in the parts array. So the 45 shouldn't be understood as the number "45", but parts[4] and parts[5]. (Which is not correct in the first place, I believe.) You can see this notation on display a little more clearly in to_shortest_exp_str, where we have [+][1][.][2345][e][-][6] as a result of this patch, though note that 1-based indexing is being used here rather than 0-based... I changed 45 to 2 following said understanding, as the non-zero numbers there would be parts[2].
I agree that this is not the best notation for explaining things! Perhaps a separate pull request would be appropriate to make the documentation a little clearer?
There was a problem hiding this comment.
… I think you are overthinking about this 😄 When the documentation was originally written, the + sign is indeed a part, so [+][1][.][2345][e][-][67] had 7 parts because there are 7 []s, not due to 1234567. Later the + sign is no longer a part but the documentation failed to catch up until this PR.
|
@sfackler Could you take a look at the changes here? You're assigned. cc @BurntSushi |
|
@bors r+ |
|
📌 Commit b185844 has been approved by |
|
⌛ Testing commit b185844 with merge 788c960... |
|
💔 Test failed - status-appveyor |
fix confusion about parts required for float formatting The documentation for flt2dec doesn't match up with the actual implementation, so fix the documentation to align with reality. Presumably due to the mismatch, the formatting code for floats in std::fmt can use correspondingly shorter arrays in some places, so fix those places up as well. Fixes rust-lang#41304.
fix confusion about parts required for float formatting The documentation for flt2dec doesn't match up with the actual implementation, so fix the documentation to align with reality. Presumably due to the mismatch, the formatting code for floats in std::fmt can use correspondingly shorter arrays in some places, so fix those places up as well. Fixes #41304.
|
☀️ Test successful - status-appveyor, status-travis |
The documentation for flt2dec doesn't match up with the actual
implementation, so fix the documentation to align with reality.
Presumably due to the mismatch, the formatting code for floats in
std::fmt can use correspondingly shorter arrays in some places, so fix
those places up as well.
Fixes #41304.