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

1. 
   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

2. 
   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

   Reviewers, this pull request conflicts with the following ones:

   • #32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)

   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.

3. DrahtBot added the label Utils/log/libs on May 15, 2025
4. util: C++20 `ToIntegral()` Improvement 6135cc3f07
5. w0xlt force-pushed on May 16, 2025
6. 
   shahsb commented at 4:56 am on May 16, 2025: none

   Concept ACK

   Approach ACK

7. 
   shahsb commented at 4:57 am on May 16, 2025: none

   LGTM. Indeed, good improvement.

   Thanks for making the changes.

8. 
   shahsb commented at 4:57 am on May 16, 2025: none

   ACK https://github.com/bitcoin/bitcoin/pull/32522/commits/6135cc3f07ae91cab91927a2b610aaed2fb38858
9. 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.)
10. 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.

Contributors
w0xlt DrahtBot shahsb maflcko

Labels
Utils/log/libs