-
Notifications
You must be signed in to change notification settings - Fork 185
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
Add exponential formatting to BigInt #214
base: master
Are you sure you want to change the base?
Conversation
src/biguint.rs
Outdated
} else { | ||
// The max digits that can fit into a f64, which ensures | ||
// that printing won't have rounding errors. | ||
const MAX_FLOAT_DIGITS: usize = 14; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f64::DIGITS
is 15, though it does say "approximate". Perhaps we should us that, minus one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I subtracted 1 to ensure the rounding wouldn't turn out weird, but forgot to update the docs. I didn't know f64::DIGITS
existed, so I'll update to take that into account.
buffer.push('.'); | ||
for &ch in iter.take(precision) { | ||
buffer.push(ch as char); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need rounding if there were more digits than the precision required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Added.
} | ||
buffer.push(if upper { 'E' } else { 'e' }); | ||
write!(buffer, "{}", digits.len() - 1)?; | ||
f.pad_integral(is_nonneg, "", &buffer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pad_integral
feels weird in this case, but I'm not sure it's wrong. The standard library uses its internal pad_formatted_parts
for exponential integers, but at a glance I don't see what would be different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely weird, but it does seem to do the right thing. I've added a comment to that effect.
Would this PR be merged soon? I'm looking for similar functionality on this. |
Should close #6.