SaturatingAdd() and AdditionOverflow() in src/util/overflow.hThese changes increase the type safety of the code and were done while exploring increasing the type-safety of CAmount (currently just a typedef of int64_t).
The first commit has nothing to do with overflow but is along for the ride if reviewers agree.
Broken out from larger effort to make CAmount more type safe.
Previously we could fall back to using an unspecialized implementation of std::numeric_limits<T> which would compile as long as the numeric operators existed, but would return 0 for min() & max().
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35372.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, winterrdog, sedited |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Concept ACK
14 | @@ -15,6 +15,7 @@ 15 | 16 | template <std::integral T> 17 | [[nodiscard]] bool AdditionOverflow(const T i, const T j) noexcept 18 | + requires(!std::is_same_v<T, bool>)
Instead of adding requires to each utility, would it be preferable to define a simple concept that is std::integral but without the bool type (ditto std::unsigned_integral)?
template <typename T>
concept non_bool_integral =
std::integral<std::remove_cvref_t<T>> &&
!std::same_as<std::remove_cvref_t<T>, bool>;
agree with this direction
i wonder if it may be even cleaner to keep the bool exclusion itself as the reusable/composable concept, instead of coupling it directly into non_bool_integral.
sth like:
template <typename T>
concept not_bool =
!std::same_as<std::remove_cvref_t<T>, bool>;
then compose it where needed, for instance:
requires std::integral<T> && not_bool<T>
the reason i slightly prefer this direction is that not_bool feels like an orthogonal semantic property rather than something inherently tied to integrals specifically.
it also scales a bit better compositionally since some call sites may only care about excluding bool, while others may want combinations like:
std::unsigned_integral<T> && not_bool<T>
or other constraints later on.
that way the concepts can stay small and reusable, while the constraints read closer to their actual intent.
keen to hear what you think about this approach.
Thanks for the feedback and example code. While it does feel weird to allow booleans here, I think I'll save that battle for another time if I get stronger ammunition for advocating it.
approach ACK
using c++20 concepts here is a good move to enforce strict type safety
34 | @@ -35,14 +35,16 @@ template <class T> 35 | 36 | template <std::unsigned_integral T, std::unsigned_integral U> 37 | [[nodiscard]] constexpr bool TrySub(T& i, const U j) noexcept 38 | + requires(!std::is_same_v<T, bool> && !std::is_same_v<U, bool>)
Seems fine, but is there anything wrong here? Looking at the diff, using bool seems used and perfectly fine:
- Assume(TrySub(m_dirty_count, it->second.IsDirty()));
+ Assume(TrySub(m_dirty_count, unsigned{it->second.IsDirty()}));
And about the other changes in this file: I can't really see bool to be used here in a buggy way, and if there was an unlikely case where it really was used, it would fail at runtime.
No objection, but all of this seems a bit verbose, absent a real (past) bug or imaginary bug that this could be catching?
Fair. Deferring disallowing booleans for now.
Latest push removes the last commit disallowing booleans in overflow.h. Should be less controversial.
lgtm ACK 0774eaaf0c221b3fed68e866130abafa2890880c
ACK 0774eaaf0c221b3fed68e866130abafa2890880c
the updated diff drops the custom constraints that disallowed booleans
ACK 0774eaaf0c221b3fed68e866130abafa2890880c