util: Make Parse{Int,UInt}{32,64} use locale independent std::from_chars(…) (C++17) instead of locale dependent strto{l,ll,ul,ull}

[practicalswift]

Make Parse{Int,UInt}{32,64} use locale independent std::from_chars(…) (C++17) instead of locale dependent strto{l,ll,ul,ull}.

About std::from_chars: "Unlike other parsing functions in C++ and C libraries, std::from_chars is locale-independent, non-allocating, and non-throwing."

[laanwj]

Concept ACK, see also discussion in #20452 (comment)

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #20452 (util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[practicalswift]

Like #20452 this one is blocked on #20460. That's the reason for the CI failures :)

[practicalswift]

Could remove "Waiting for author"? I don't know if we have any "Blocked on another PR/issue" (or "Waiting for toolchain upgrade") tag, but this PR is blocked on #20460 :)

[jonatack]

Concept ACK. Looks very good; hope to see this unblocked with std::fs.

[practicalswift]

Now that #20460 has been merged I think this PR should be ready for final review :)

See also related PR #20452.

[laanwj]

Nice!
Code review ACK 4747db8

[maflcko]

Can you explain what the { are supposed to do? I think they can be removed, as they are confusing as-is. If you want them to check (for whatever reason) that result fits in type T, you will have to type return T{result};

[practicalswift]

In this specific case return result; should be equivalent to return {result}; since the return type is std::optional<T> and result is of type T. I can see your point about braces looking a bit odd here: feel free to change :)

Some context behind the "braced return" style more generally: it can help avoid nasty implicit narrowing conversions like in the following example.

$ cat without-list-initialization.cpp
#include <cstdint>

struct F {
  F(uint8_t) {}
};

F f() { return 1024; }
$ clang++ -c without-list-initialization.cpp
# No errors
$ echo $?
0

$ cat with-list-initialization.cpp
#include <cstdint>

struct F {
  F(uint8_t) {}
};

F f() { return {1024}; }
$ clang++ -c with-list-initialization.cpp
with-list-initialization.cpp:7:18: error: constant expression evaluates to 1024 which cannot be narrowed to type 'uint8_t' (aka 'unsigned char') [-Wc++11-narrowing]
$ echo $?
1

See item (8) and "narrowing conversions" in https://en.cppreference.com/w/cpp/language/list_initialization :)

[practicalswift]

FWIW also: https://clang.llvm.org/extra/clang-tidy/checks/modernize-return-braced-init-list.html

[maflcko]

FWIW also: https://clang.llvm.org/extra/clang-tidy/checks/modernize-return-braced-init-list.html

return {baz}; fails when explicit Foo(Baz)

[maflcko]

crACK

[maflcko]

I am wondering if it makes sense to explain in simple terms the format that is required to be parsed successfully. In regex terms it would be -?[0-9]+?

[maflcko]

Created a follow-up in #23156

[rebroad]

  CXX      qt/qt_bitcoin_qt-main.o
In file included from qt/main.cpp:5:
In file included from ./qt/bitcoin.h:12:
In file included from ./interfaces/node.h:10:
In file included from ./net.h:9:
In file included from ./addrman.h:9:
In file included from ./netaddress.h:19:
./util/strencodings.h:16:10: fatal error: 'charconv' file not found
#include <charconv>
         ^~~~~~~~~~
1 error generated.
Makefile:12673: recipe for target 'qt/qt_bitcoin_qt-main.o' failed
make: *** [qt/qt_bitcoin_qt-main.o] Error 1

[sipa]

@rebroad Bizarre, sounds like you're not using a C++17 compliant compiler.

[rebroad]

@rebroad Bizarre, sounds like you're not using a C++17 compliant compiler.

Still getting the same error when compiling in a clean directory... could it be an issue caused by ccache?

[rebroad]

4747db876154ddd828c03d9eda10ecf8b25d8dc8 is the first bad commit
commit 4747db876154ddd828c03d9eda10ecf8b25d8dc8
Author: practicalswift <[email protected]>
Date:   Sat Sep 18 04:30:30 2021 +0000

    util: Introduce ToIntegral<T>(const std::string&) for locale independent parsing using std::from_chars(…) (C++17)

    util: Avoid locale dependent functions strtol/strtoll/strtoul/strtoull in ParseInt32/ParseInt64/ParseUInt32/ParseUInt64

    fuzz: Assert equivalence between new and old Parse{Int,Uint}{8,32,64} functions

    test: Add unit tests for ToIntegral<T>(const std::string&)

:040000 040000 ec09e7aa0a2bec6f5021c2bf2994a0fc67e65c1f aa5170295604fa240939c86c02f26a6e436be964 M      src
:040000 040000 aa260b3d4bb2c554e68a7b0baa241a2cfc97818a c5d3df2e7337c8cc6a5a2141844b5c1b81a6ba51 M      test