Skip to content

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

Merged
StephanTLavavej merged 2 commits intomicrosoft:masterfrom
statementreply:fix_gh_931
Aug 18, 2020
Merged

<charconv>: fix from_chars("0.fffffffffffff8p-1022")#934
StephanTLavavej merged 2 commits intomicrosoft:masterfrom
statementreply:fix_gh_931

Conversation

@statementreply
Copy link
Contributor

@statementreply statementreply commented Jun 28, 2020

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

StephanTLavavej commented Jul 3, 2020

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

StephanTLavavej commented Aug 18, 2020

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

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+1 more reviewer

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

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