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."
Concept ACK, see also discussion in #20452 (comment)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
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 :)
Concept ACK. Looks very good; hope to see this unblocked with std::fs.
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&)Nice! Code review ACK 4747db876154ddd828c03d9eda10ecf8b25d8dc8
109 | + T result; 110 | + const auto [first_nonmatching, error_condition] = std::from_chars(str.data(), str.data() + str.size(), result); 111 | + if (first_nonmatching != str.data() + str.size() || error_condition != std::errc{}) { 112 | + return std::nullopt; 113 | + } 114 | + return {result};
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};
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 :)
FWIW also: https://clang.llvm.org/extra/clang-tidy/checks/modernize-return-braced-init-list.html
return {baz}; fails when explicit Foo(Baz)
crACK
101 | + * 102 | + * @returns std::nullopt if the entire string could not be parsed, or if the 103 | + * parsed value is not in the range representable by the type T. 104 | + */ 105 | +template <typename T> 106 | +std::optional<T> ToIntegral(const std::string& str)
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]+?
Created a follow-up in #23156
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
4747db876154ddd828c03d9eda10ecf8b25d8dc8 is the first bad commit
commit 4747db876154ddd828c03d9eda10ecf8b25d8dc8
Author: practicalswift <practicalswift@users.noreply.github.com>
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
Milestone
23.0