util: C++20 `ToIntegral()` Improvement #32522

pull w0xlt wants to merge 1 commits into bitcoin:master from w0xlt:to_integral changing 1 files +5 −4

w0xlt commented at 11:51 PM on May 15, 2025: contributor
While reviewing #32520, I noticed that the function ToIntegral() could be improved:
- Instead of manually asserting std::is_integral_v<T>, the template can be constrained with the standard std::integral for clearer intent.
- Annotation with [[nodiscard]] and noexcept. Compilers will warn if the return value is discarded and since std::from_chars is guaranteed to throw nothing, declaring the wrapper noexcept makes the exception guarantee explicit.
- With C++23, the integral overloads of std::from_chars are constexpr, allowing ToIntegral to become a constexpr function.
https://en.cppreference.com/w/cpp/utility/from_chars
DrahtBot commented at 11:51 PM on May 15, 2025: contributor



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



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32522.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK shahsb

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

No conflicts as of last run.
DrahtBot added the label Utils/log/libs on May 15, 2025
util: C++20 `ToIntegral()` Improvement 6135cc3f07
w0xlt force-pushed on May 16, 2025
shahsb commented at 4:56 AM on May 16, 2025: none

Concept ACK

Approach ACK
shahsb commented at 4:57 AM on May 16, 2025: none

LGTM. Indeed, good improvement.

Thanks for making the changes.
shahsb commented at 4:57 AM on May 16, 2025: none

ACK https://github.com/bitcoin/bitcoin/pull/32522/commits/6135cc3f07ae91cab91927a2b610aaed2fb38858
in src/util/strencodings.h:183 in 6135cc3f07
```
 182 | +template <std::integral T> requires (!std::same_as<T,bool>)
 183 | +[[nodiscard]] constexpr std::optional<T> ToIntegral(std::string_view str) noexcept
 184 |  {
 185 | -    static_assert(std::is_integral_v<T>);
 186 | -    T result;
 187 | +    T result{};
```
maflcko commented at 6:00 AM on May 16, 2025:

not sure. We actually want C++26 erroneous behavior here, not [[indeterminate]], nor (zero-)initialized. Otherwise, it will become harder to diagnose bugs in the code with the compiler or with sanitizers.

maflcko commented at 6:13 AM on May 16, 2025:

(realistically speaking in this very instance it probably doesn't make a difference and anything is fine, I just wanted to mention it for the general context.)

fanquake commented at 2:10 PM on July 2, 2025:

Otherwise, it will become harder to diagnose bugs in the code with the compiler or with sanitizers. @dergoegge any opinion?
in src/util/strencodings.h:176 in 6135cc3f07
```
 171 | @@ -172,14 +172,15 @@ constexpr inline bool IsSpace(char c) noexcept {
 172 |   * trailing character fail the parsing. The required format expressed as regex
 173 |   * is `-?[0-9]+`. The minus sign is only permitted for signed integer types.
 174 |   *
 175 | + * @tparam T Integral type (excluding bool).
 176 | + * @param str The input view to parse.
```
maflcko commented at 6:04 AM on May 16, 2025:

not sure about adding docs that are already self-explanatory from the code without doubt. Adding such docs just means that two lines will have to be modified when modifying a trivial single line of code. Also, if the docs aren't needed, they are only a source of inconsistencies and typos, etc.
w0xlt closed this on Aug 5, 2025