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.

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