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.

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

<charconv>: fix from_chars("0.fffffffffffff8p-1022") #934

Merged
merged 2 commits into from
Aug 18, 2020
Merged

<charconv>: fix from_chars("0.fffffffffffff8p-1022") #934

merged 2 commits into from
Aug 18, 2020

Conversation

statementreply
Copy link
Contributor

This PR fixes a bug in the computation of floating point exponent for a special case in from_chars, when the magnitude of the result before rounding is <= 0.5 ulp smaller than numeric_limits<T>::min(), and after rounding it becomes a normal number.

Fixes #931

@statementreply
Add test cases for microsoft#931
ef5c84f
@statementreply
Fix microsoft#931: hex from_chars("0.fffffffffffff8p-1022") is incorrect
10041f1 
This commit fixes a bug the computation of floating point exponent when
the magnitude of the result before rounding is <= 0.5 ulp smaller than
numeric_limits<T>::min(), and after rounding it becomes a normal number.
@statementreply statementreply requested a review from a team as a code owner June 28, 2020 15:38
@statementreply
Copy link
Contributor Author

statementreply commented Jun 28, 2020 •
edited
Loading

Side note: It appears to me that _Assemble_floating_point_value could be simplified. We could compute ((exponent + bias - 1) << exponent_shift) + significand_with_msb instead of ((exponent + bias) << exponent_shift) | significand_without_msb, and it just works even when the value is subnormal or when renormalization is required.

@CaseyCarter CaseyCarter added the bug Something isn't working label Jun 28, 2020
@StephanTLavavej StephanTLavavej self-assigned this Jun 30, 2020
@miscco miscco mentioned this pull request Jul 2, 2020
Merged
@StephanTLavavej
Copy link
Member

Current status: after the US holiday this weekend, I'll need to work on a bit of spaceship stuff to unblock other maintainers, then I should have time to review this (which is mostly a matter of exploring how the fix works and getting my brain to catch up - if I had written this part from scratch I would understand it immediately).

StephanTLavavej
StephanTLavavej approved these changes Aug 14, 2020
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good to me, including the set of test cases. Thanks!

@StephanTLavavej StephanTLavavej removed their assignment Aug 14, 2020
CaseyCarter
CaseyCarter approved these changes Aug 15, 2020
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

I really hope we never lose the comments in <charconv>.

@CaseyCarter CaseyCarter removed their assignment Aug 15, 2020
@StephanTLavavej StephanTLavavej merged commit 9fb6caa into microsoft:master Aug 18, 2020
@StephanTLavavej
Copy link
Member

Thanks for finding and fixing this correctness bug! This will ship in VS 2019 16.8 Preview 3. 😸

@statementreply statementreply deleted the fix_gh_931 branch April 17, 2021 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

<charconv>: hex from_chars("0.fffffffffffff8p-1022") is incorrect
4 participants
@statementreply @StephanTLavavej @CaseyCarter @barcharcraz