util: Add SaturatingAdd helper #24224

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2201-satadd changing 3 files +48 −5

MarcoFalke commented at 12:49 PM on February 1, 2022: member
Seems good to have this in the repo, as it might be needed to write cleaner code. For example:
- #24090 (comment)
- #23418 (review)
- ...
DrahtBot added the label Utils/log/libs on Feb 1, 2022
laanwj commented at 11:08 AM on February 3, 2022: member

Concept ACK. This seems useful, hopefully C++ will have built-ins for explicitly saturating/wrapping/checked arithmetic like rust some day. But until then…
MarcoFalke commented at 12:35 PM on February 3, 2022: member

saturating/wrapping/checked arithmetic like rust

I wasn't aware that rust has those built-in. Mind sharing a link to the docs?
MarcoFalke commented at 12:41 PM on February 3, 2022: member
Oh, I see. By default rust will panic, but there are https://doc.rust-lang.org/std/num/index.html:
- https://doc.rust-lang.org/std/num/struct.Saturating.html
- https://doc.rust-lang.org/std/num/struct.Wrapping.html
in src/util/overflow.h:33 in fa90189cbf outdated
```
  27 | @@ -28,4 +28,22 @@ template <class T>
  28 |      return i + j;
  29 |  }
  30 |  
  31 | +template <class T>
  32 | +[[nodiscard]] T SaturatingAdd(const T i, const T j) noexcept
```
klementtan commented at 4:52 AM on February 12, 2022:

Out of curiosity, any reason you didn't constexpr this function?

MarcoFalke commented at 7:09 AM on February 12, 2022:

Outside of unit tests I don't think this will ever be called in a context that would be constexpr.
klementtan approved
klementtan commented at 5:14 AM on February 12, 2022: contributor

Code Review ACK fa90189cbfe10552f94fdaa8607cdd5a10341ced

in src/test/util_tests.cpp:1478 in fa90189cbf outdated

1472 | @@ -1473,9 +1473,15 @@ static void TestAddMatrixOverflow()
1473 |      constexpr T MAXI{std::numeric_limits<T>::max()};
1474 |      BOOST_CHECK(!CheckedAdd(T{1}, MAXI));
1475 |      BOOST_CHECK(!CheckedAdd(MAXI, MAXI));
1476 | +    BOOST_CHECK_EQUAL(MAXI, SaturatingAdd(T{1}, MAXI));
1477 | +    BOOST_CHECK_EQUAL(MAXI, SaturatingAdd(MAXI, MAXI));

vasild commented at 12:52 PM on February 21, 2022:

Maybe add a test like:

BOOST_CHECK_EQUAL(MAXI - 1, SaturatingAdd(T{1}, MAXI - 2));

to ensure that SaturatingAdd() does not always, or too often, return MAXI. Or, keeps being accurate close to the limit.

(same for MINI + 1 tests below)

MarcoFalke commented at 1:33 PM on February 21, 2022:

thx, done

vasild approved
vasild commented at 12:54 PM on February 21, 2022: member

ACK fa90189cbfe10552f94fdaa8607cdd5a10341ced
util: Add SaturatingAdd helper faa7d8a3f7
MarcoFalke force-pushed on Feb 21, 2022
MarcoFalke commented at 1:34 PM on February 21, 2022: member

Added a test. Should be trivial to re-ACK with git range-diff bitcoin-core/master fa90189cbf faa7d8a3f7
vasild approved
vasild commented at 2:21 PM on February 21, 2022: member

ACK faa7d8a3f7cba02eca7e247108a6b98ea9daf373
klementtan commented at 2:34 PM on February 21, 2022: contributor

reACK faa7d8a3f7
MarcoFalke merged this on Feb 21, 2022
MarcoFalke closed this on Feb 21, 2022
MarcoFalke deleted the branch on Feb 21, 2022
sidhujag referenced this in commit deb823f097 on Feb 22, 2022
DrahtBot locked this on Feb 21, 2023

Contributors

Labels

Linked (view graph)

#23418 Fix signed integer overflow in prioritisetransaction RPC #24090 net/p2p: change nScore and nBestScore data types to int64_t