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.

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

Fix num_get's float-parsing problem #3982

Merged
merged 1 commit into from Sep 7, 2023

Conversation

achabense
Copy link
Contributor

@achabense achabense commented Aug 20, 2023

Fixes #3980 / DevCom-10450662 / VSO-1876474 / AB#1876474

Also fixes DevCom-10445082 / VSO-1873604 / AB#1873604.

Likely fixes VSO-1874745 / AB#1874745.

Likely fixes VSO-1876601 / AB#1876601.

@achabense achabense requested a review from a team as a code owner August 20, 2023 11:19
@github-actions github-actions bot added this to Initial Review in Code Reviews Aug 20, 2023
@CaseyCarter CaseyCarter added the bug Something isn't working label Aug 23, 2023
@StephanTLavavej StephanTLavavej self-assigned this Sep 2, 2023
@StephanTLavavej StephanTLavavej added the high priority Important! label Sep 5, 2023
@achabense
Copy link
Contributor Author

achabense commented Sep 6, 2023

I haven't checked the standard carefully, but please check whether this is just another problem:
If the function sees exponent character and finds no following digits, it will consider the whole section as invalid:
(maybe not a problem, as gcc and clang have the same output)

#include <iostream>
#include <sstream>
using namespace std;

int main()
{
    istringstream istr("1.3e");
    double x = 0;
    istr >> x;

    double y = stod("1.3e");
    cout << x << ' ' << y; // "0 1.3"
}

Relevant logic:
https://github.com/microsoft/STL/blob/6c69a73911b33892919ec628c0ea5bbf0caf8a6a/stl/inc/xlocnum#L1003-L1006r
https://github.com/microsoft/STL/blob/6c69a73911b33892919ec628c0ea5bbf0caf8a6a/stl/inc/xlocnum#L1042-L1044r

@StephanTLavavej StephanTLavavej removed their assignment Sep 7, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Sep 7, 2023
@StephanTLavavej StephanTLavavej self-assigned this Sep 7, 2023
@StephanTLavavej
Copy link
Member

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@statementreply
Copy link
Contributor

statementreply commented Sep 7, 2023

I haven't checked the standard carefully, but please check whether this is just another problem: If the function sees exponent character and finds no following digits, it will consider the whole section as invalid: (maybe not a problem, as gcc and clang have the same output)

I think this is the correct behavior according to [facet.num.get.virtuals]/3.3.6:

The numeric value to be stored can be one of:

  • zero, if the conversion function does not convert the entire field.
  • ...

The resultant numeric value is stored in val. If the conversion function does not convert the entire field, or if the field represents a value outside the range of representable values, ios_base​::​failbit is assigned to err.

@achabense
Copy link
Contributor Author

achabense commented Sep 7, 2023

For the tests introduced in this pr, clang accepts all and gcc is (wrongly?) rejecting those with 0x prefixes.

@StephanTLavavej
Copy link
Member

#3364 implemented LWG-2381 which taught the STL to recognize p and P exponents, thus accepting hexfloats. Also see the Annex C wording added by the LWG issue resolution that explains this change.

Copy link
Member

@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.

Just style nitpicks; ignore me.

// Also test GH-3980 <iostream>: Wrong reading of float values
template <class Flt>
void test_gh_3980() {
auto test_case = [](const char* str, double expected) {
Copy link
Member

Choose a reason for hiding this comment

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

Not worth resetting testing: there are a log of strs floating around here, it might be nice to call this parameter "input" or something.

@@ -532,6 +532,23 @@ void test_gh_3378() {
}
}

// Also test GH-3980 <iostream>: Wrong reading of float values
template <class Flt>
Copy link
Member

Choose a reason for hiding this comment

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

Not worth resetting testing: vwls pls.
Flt isn't that much shorter than Float. If you want a descriptive name, be descriptive. If you want a short name, T would work fine here and honestly std::floating_point T (or static_assert(std::is_floating_point_v<T>) in the function body) would be even better.

Copy link
Member

Choose a reason for hiding this comment

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

Float specifically is dangerous due to the potential confusion with float. Floating or any other name avoids that hazard.

@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Sep 7, 2023
@StephanTLavavej StephanTLavavej merged commit f98b286 into microsoft:main Sep 7, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Sep 7, 2023
@StephanTLavavej
Copy link
Member

Thanks for fixing this major regression! 🛠️ ✨ 🎉

@achabense achabense deleted the _GH3980_fix branch September 9, 2023 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority Important!
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

<iostream>: Wrong reading of float values
4 participants