util: C++20 ToIntegral() Improvement
#32522
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32522. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
| { | ||
| static_assert(std::is_integral_v<T>); | ||
| T result; | ||
| T result{}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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.)
There was a problem hiding this comment.
Otherwise, it will become harder to diagnose bugs in the code with the compiler or with sanitizers.
@dergoegge any opinion?
| * @tparam T Integral type (excluding bool). | ||
| * @param str The input view to parse. |
There was a problem hiding this comment.
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.
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 standardstd::integralfor clearer intent.Annotation with
[[nodiscard]]andnoexcept. Compilers will warn if the return value is discarded and sincestd::from_charsis guaranteed to throw nothing, declaring the wrappernoexceptmakes the exception guarantee explicit.With C++23, the integral overloads of
std::from_charsareconstexpr, allowingToIntegralto become aconstexprfunction.https://en.cppreference.com/w/cpp/utility/from_chars