Bug 105441 - [12/13 Regression] The floating point overload of from_chars ignores 'P' for hex format
Summary: [12/13 Regression] The floating point overload of from_chars ignores 'P' for ...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 12.0
: P3 normal
Target Milestone: 12.0
Assignee: Patrick Palka
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-30 16:24 UTC by 康桓瑋
Modified: 2022-05-03 13:23 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 12.0, 13.0
Last reconfirmed: 2022-04-30 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 康桓瑋 2022-04-30 17:35:48 UTC
floating_from_chars.cc#L667

// Parse the written exponent.
int written_exponent = 0;
if (first != last && *first == 'p')
  {
    // Tentatively consume the 'p' and try to parse a decimal number.
    const char* const fallback_first = first;

it seems like it should be

if (first != last && std::tolower((unsigned char)*first) == 'p')
Comment 2 康桓瑋 2022-04-30 17:36:03 UTC
floating_from_chars.cc#L667

// Parse the written exponent.
int written_exponent = 0;
if (first != last && *first == 'p')
  {
    // Tentatively consume the 'p' and try to parse a decimal number.
    const char* const fallback_first = first;

it seems like it should be

if (first != last && std::tolower((unsigned char)*first) == 'p')
Comment 3 Patrick Palka 2022-04-30 17:50:55 UTC
Thanks for reporting and analyzing this!

(In reply to 康桓瑋 from comment #2)
> floating_from_chars.cc#L667
> 
> // Parse the written exponent.
> int written_exponent = 0;
> if (first != last && *first == 'p')
>   {
>     // Tentatively consume the 'p' and try to parse a decimal number.
>     const char* const fallback_first = first;
> 
> it seems like it should be
> 
> if (first != last && std::tolower((unsigned char)*first) == 'p')

I'm not sure we can use tolower here because it's locale dependent and charconv isn't.  So we should just test for 'p' and 'P' directly.  And we should probably replace the existing use of tolower in find_end_of_float.

I wonder why the SO user is seeing this bug with GCC 11.2?  The new hexfloat parser (r12-6645-gcc3bf3404e4b1c) is GCC 12 only.
Comment 4 GCC Commits 2022-05-02 11:03:30 UTC
The master branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

https://gcc.gnu.org/g:576f975cabb0fd9843de152a2d247d486a967b08

commit r13-69-g576f975cabb0fd9843de152a2d247d486a967b08
Author: Patrick Palka <ppalka@redhat.com>
Date:   Mon May 2 07:00:48 2022 -0400

    libstdc++: case-sensitivity in hexfloat std::from_chars [PR105441]
    
    The hexfloat parser for binary32/64 added in r12-6645-gcc3bf3404e4b1c
    overlooked that the exponent part can also begin with an uppercase 'P'.
    
            PR libstdc++/105441
    
    libstdc++-v3/ChangeLog:
    
            * src/c++17/floating_from_chars.cc (__floating_from_chars_hex):
            Also accept 'P' as the start of the exponent.
            * testsuite/20_util/from_chars/7.cc: Add corresponding testcase.
Comment 5 GCC Commits 2022-05-02 11:44:45 UTC
The releases/gcc-12 branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

https://gcc.gnu.org/g:4a6d7da796e456115bbac92e056123f095a3780c

commit r12-8327-g4a6d7da796e456115bbac92e056123f095a3780c
Author: Patrick Palka <ppalka@redhat.com>
Date:   Mon May 2 07:00:48 2022 -0400

    libstdc++: case-sensitivity in hexfloat std::from_chars [PR105441]
    
    The hexfloat parser for binary32/64 added in r12-6645-gcc3bf3404e4b1c
    overlooked that the exponent part can also begin with an uppercase 'P'.
    
            PR libstdc++/105441
    
    libstdc++-v3/ChangeLog:
    
            * src/c++17/floating_from_chars.cc (__floating_from_chars_hex):
            Also accept 'P' as the start of the exponent.
            * testsuite/20_util/from_chars/7.cc: Add corresponding testcase.
    
    (cherry picked from commit 576f975cabb0fd9843de152a2d247d486a967b08)
Comment 6 Patrick Palka 2022-05-03 13:23:19 UTC
Should be fixed.

Replacing the use of std::tolower was done in the subsequent commit r13-70-g86d821ddf5615e.