Skip to content

flt2dec comments and code disagree about the numbers of parts required for formatting #41304

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.

Sign up for GitHub

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

Jump to bottom
Closed
froydnj opened this issue Apr 14, 2017 · 0 comments · Fixed by #41859
Closed

flt2dec comments and code disagree about the numbers of parts required for formatting #41304

froydnj opened this issue Apr 14, 2017 · 0 comments · Fixed by #41859

Comments

@froydnj
Copy link
Contributor

froydnj commented Apr 14, 2017

The comments for flt2dec::to_shortest and other similar functions describe the required size of the &mut [Part] parameters. However, the assertions contained in those functions do not check the size described in the comments:

  • to_shortest_str requires 5 in the comments, but asserts that the size is >= 4;
  • to_exact_fixed_str does the same.

The comments appear to be confused, as the [0.] bit that they are apparently counting as a Part is not; perhaps this is a remnant left over from a previous refactoring.

  • to_shortest_exp_str requires 7 in the comments, but asserts that the size is >= 6;
  • to_exact_exp_str does the same.

The comments suggest that the worst case for the exponent would require 17 bits or more, which seems overly conservative.
froydnj added a commit to froydnj/rust that referenced this issue May 9, 2017
@froydnj
fix confusion about parts required for float formatting
b185844 
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.
@froydnj froydnj mentioned this issue May 9, 2017
Merged
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue May 14, 2017
@Mark-Simulacrum
Rollup merge of rust-lang#41859 - froydnj:align-float-parts, r=sfackler
547b978 
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.
bors added a commit that referenced this issue May 15, 2017
@bors
Auto merge of #41859 - froydnj:align-float-parts, r=sfackler
ac254fb 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix confusion about parts required for float formatting froydnj/rust
1 participant
@froydnj