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

Implement LWG-2381 Inconsistency in parsing floating point numbers #3364

Merged
merged 67 commits into from May 5, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Jan 27, 2023

Fixes #2388. Fixes #1582. Fixes #3375. Fixes #3376. Fixes #3378.

Also works towards

And updates references to working draft in <xlocnum> to WG21-N4944.

The new test file is derived from libc++'s test files, except that "inf"/"nan" cases are skipped. It currently seems non-conforming to handle "inf"/"nan" with do_get.

This PR also (possibly) fixes two six pre-existing bugs:

  1. std::hexfloat should have no effect on parsing floating-point numbers.
  2. When the input sequence represents an overly large but valid floating-point value, e.g., "1.0e320" for double, infinity (or negative infinity) instead of 0.0 should be given.
  3. (Thanks to @strega-nil-ms ) do_get should not consider CHAR_MAX or a non-positive char value in grouping() ends the grouping, since it means that the current group is unlimited, but the next group may still be limited.
  4. (Thanks to @statementreply ) "9.999 .... [1000 nines] ...9" should be parsed as 10, not 100.
  5. (Thanks to @statementreply ) The _Sticky logic should also be applied to hexadecimal representations.
  6. (Thanks to @statementreply ) The carrying logic should be less greedy - but it turns out that carrying can be removed, so this PR removes it.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner January 27, 2023 10:09
@CaseyCarter CaseyCarter added the LWG Library Working Group issue label Jan 27, 2023
@CaseyCarter CaseyCarter added this to Initial Review in Code Reviews via automation Jan 27, 2023
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

That's what I see -- thanks for fixing these bugs in this (fairly crufty!) code 😸

tests/std/tests/LWG2318_num_get_floating_point/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/LWG2318_num_get_floating_point/test.cpp Outdated Show resolved Hide resolved
stl/inc/xlocnum Outdated Show resolved Hide resolved
stl/inc/xlocnum Outdated Show resolved Hide resolved
stl/inc/xlocnum Outdated Show resolved Hide resolved
stl/inc/xlocnum Outdated Show resolved Hide resolved
Code Reviews automation moved this from Initial Review to Work In Progress Jan 27, 2023
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Initial Review in Code Reviews Jan 29, 2023
- eliminate some Yoda conditions
- reduce the scope of `_Sticky`
- compactify the use of `_Has_dec_leading_zero`
@statementreply

This comment was marked as resolved.

@frederick-vs-ja

This comment was marked as resolved.

Co-authored-by: statementreply <statementreply@gmail.com>
stl/inc/xlocnum Outdated Show resolved Hide resolved
stl/inc/xlocnum Outdated Show resolved Hide resolved
stl/inc/xlocnum Outdated Show resolved Hide resolved
stl/inc/xlocnum Outdated Show resolved Hide resolved
stl/inc/xlocnum Show resolved Hide resolved
tests/std/tests/LWG2381_num_get_floating_point/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/LWG2381_num_get_floating_point/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/LWG2381_num_get_floating_point/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Apr 26, 2023
@StephanTLavavej
Copy link
Member

Thanks again for this amazing work which I would not have been able to accomplish! 😻 I pushed minor nitpicky changes, nothing substantial (FYI @strega-nil-ms anyways).

With the addition of the charconv test cases, and the "if it doesn't match, at least it should match strtod/strtof" workaround, I am much more confident that we won't be damaging correctness here. Thanks for making your test changes totally non-intrusive too!

The change is still higher risk than most changes, but I believe we're preserving ABI here, and this is a significant improvement over the status quo. Let's do it.

@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Apr 26, 2023
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Apr 26, 2023
@StephanTLavavej StephanTLavavej self-assigned this Apr 27, 2023
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej
Copy link
Member

I've had to push an additional commit to restore the dllexport surface.

_Parse_fp_with_locale() needs to be templated, otherwise it'll be dllexported when the whole class is.

_Getffld() and _Getffldx() needed special treatment - their static constexpr char _Src[] arrays were implicitly dllexported, which shows up separately from the functions themselves. Accordingly, we need to restore the arrays even though they're unused, and we need to prevent the compiler eliding them, so I'm storing the address of the first element in a top-level volatile pointer, which is sufficient to keep the array in the DLL. (This does not work without volatile.)

@StephanTLavavej StephanTLavavej removed their assignment May 2, 2023
@StephanTLavavej
Copy link
Member

@frederick-vs-ja The compiler back-end's internal test suite (specifically spec2k eon) found a bug in this PR:

D:\GitHub\STL\out\x64>type meow.cpp
#include <cstdio>
#include <sstream>
using namespace std;

int main() {
    printf("_MSVC_STL_UPDATE: %d\n", static_cast<int>(_MSVC_STL_UPDATE));
    istringstream iss{"0\n"};
    float flt = 1729.0f;
    iss >> flt;
    if (iss) {
        printf("flt: %.1000g\n", flt);
    } else {
        printf("Parsing failure!\n");
    }
}

Before this PR (VS 2022 17.6 Preview 5):

D:\GitHub\STL\out\x64>cl /EHsc /nologo /W4 /MTd /Od meow.cpp && meow
meow.cpp
_MSVC_STL_UPDATE: 202302
flt: 0

After this PR:

D:\GitHub\STL\out\x64>cl /EHsc /nologo /W4 /MTd /Od meow.cpp && meow
meow.cpp
_MSVC_STL_UPDATE: 202304
Parsing failure!

We definitely need test coverage for this scenario.

@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Work In Progress in Code Reviews May 2, 2023
@StephanTLavavej StephanTLavavej self-assigned this May 3, 2023
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Final Review in Code Reviews May 3, 2023
@StephanTLavavej StephanTLavavej removed their assignment May 4, 2023
@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews May 4, 2023
@StephanTLavavej StephanTLavavej self-assigned this May 4, 2023
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 368e4b4 into microsoft:main May 5, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done May 5, 2023
@StephanTLavavej
Copy link
Member

Thank you for this absolutely massive correctness fix and LWG issue resolution, which the maintainers would not have been able to accomplish! We really appreciate it! 😻 🎉 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working LWG Library Working Group issue
Projects
No open projects
6 participants