refactor: Use C++17 std::clamp #23095

hebasto commented at 12:11 PM on September 25, 2021: member

This PR improves code readability.

Only safe cases with guarantee of no undefined behavior.

hebasto added the label Refactoring on Sep 25, 2021

DrahtBot commented at 2:40 PM on September 25, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23497 (Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces by ryanofsky)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

hebasto force-pushed on Sep 25, 2021

hebasto marked this as a draft on Sep 25, 2021

hebasto force-pushed on Sep 25, 2021

hebasto marked this as ready for review on Sep 26, 2021

theStack commented at 9:03 PM on September 26, 2021: member

Concept ACK

jarolrod commented at 2:38 AM on September 27, 2021: member

Concept ACK

std::clamp is standard in C++17 and is certainly a nice cleanup versus the existing code. Additionally, this is not a breaking change for windows builders as this is available since VS2015.

kiminuo commented at 7:43 AM on September 27, 2021: contributor

Concept ACK

I wonder whether this is a good candidate or not: https://github.com/bitcoin/bitcoin/blob/58c25bdcea9b95348e639180932b388d4bda8157/src/rpc/blockchain.cpp#L1859

hebasto commented at 8:24 AM on September 27, 2021: member

@kiminuo

I wonder whether this is a good candidate or not:

https://github.com/bitcoin/bitcoin/blob/58c25bdcea9b95348e639180932b388d4bda8157/src/rpc/blockchain.cpp#L1859

Some tests fail with this line being changed.

kiminuo commented at 8:34 AM on September 27, 2021: contributor

@kiminuo

I wonder whether this is a good candidate or not: https://github.com/bitcoin/bitcoin/blob/58c25bdcea9b95348e639180932b388d4bda8157/src/rpc/blockchain.cpp#L1859

In case you think it is a good candidate for changing to std::clamp, it would be helpful to post how you changed the line and a test failure details.

hebasto commented at 8:43 AM on September 27, 2021: member

@kiminuo

In case you think it is a good candidate for changing to std::clamp, it would be helpful to post how you changed the line and a test failure details.

~Here are the branch and tests.~

Branch: 7583dee2776b053928542cd82bf89e053cf4388d CI tests: https://cirrus-ci.com/build/4594368825786368

laanwj commented at 11:11 AM on September 27, 2021: member

Ouch-if tests fail that means the behavior is not the same. I think we should be careful here. For the C++11 transition we even had an explicit rule not to do all-over-the-place code modernization like this, it's very easy to accidentally introduce a bug. I'm all for using clamp but might be better to introduce it when code is changed anyway.

MarcoFalke commented at 11:22 AM on September 27, 2021: member

This is not a trivial replacement. With std::min/max the argument order doesn't matter (min(a,b)==min(b,a)). However, with std::clamp, the order must be exactly right and there is only one correct order.

martinus commented at 11:23 AM on September 27, 2021: contributor

I think the test has failed because std::clamp's behavior is undefined when when the lo value is bigger than the hi value. For std::clamp(blockcount, 0, pindex->nHeight - 1) this is possible when pindex->nHeight==0

the order must be exactly right and there is only one correct order.

Not only that, there can be cases where there is no order that is always correct :)

in src/checkqueue.h:114 in 82c4e71025 outdated

 108 | @@ -109,7 +109,7 @@ class CCheckQueue
 109 |                  //   all workers finish approximately simultaneously.
 110 |                  // * Try to account for idle jobs which will instantly start helping.
 111 |                  // * Don't do batches smaller than 1 (duh), or larger than nBatchSize.
 112 | -                nNow = std::max(1U, std::min(nBatchSize, (unsigned int)queue.size() / (nTotal + nIdle + 1)));
 113 | +                nNow = std::clamp<unsigned int>(queue.size() / (nTotal + nIdle + 1), 1, nBatchSize);

martinus commented at 11:31 AM on September 27, 2021:

Not sure if this can ever happen, but when nBatchSize==0 this will now have undefined behavior. Previously the result used to be 1.

hebasto commented at 11:46 AM on September 27, 2021:

Right. Would assert(nBatchSize >= 1); in the CCheckQueue constructor be enough?

martinus commented at 4:45 AM on September 28, 2021:

I'd put it right above the clamp, or not use clamp when there's a possibility that that could happen

hebasto commented at 7:58 AM on September 28, 2021:

I thought again, and put the assertion into the constructor because it is constructor's job to establish safe invariants for a class instance.

in src/init.cpp:884 in 82c4e71025 outdated

 880 | @@ -880,7 +881,7 @@ bool AppInitParameterInteraction(const ArgsManager& args)
 881 |  #else
 882 |      int fd_max = FD_SETSIZE;
 883 |  #endif
 884 | -    nMaxConnections = std::max(std::min<int>(nMaxConnections, fd_max - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE), 0);
 885 | +    nMaxConnections = std::clamp<int>(nMaxConnections, 0, fd_max - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE);

martinus commented at 11:35 AM on September 27, 2021:

If fd_max - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE is negative then nMaxConnections used to be 0, but now the result is undefined

hebasto commented at 8:00 AM on September 28, 2021:

This change dropped.

martinus changes_requested

DrahtBot added the label Needs rebase on Sep 27, 2021

hebasto force-pushed on Sep 28, 2021

hebasto commented at 7:54 AM on September 28, 2021: member

Updated 82c4e71025d0958b4066325731874863e1323a0e -> bf4838e6456fa744911a886356529656a16c19ed (pr23095.04 -> pr23095.05):

rebased
addressed comments

martinus approved

martinus commented at 8:21 AM on September 28, 2021: contributor

Code review ACK bf4838e6456fa744911a886356529656a16c19ed

DrahtBot removed the label Needs rebase on Sep 28, 2021

hebasto force-pushed on Sep 28, 2021

hebasto commented at 8:24 AM on September 29, 2021: member

Updated bf4838e6456fa744911a886356529656a16c19ed -> 665353fa65da110a9bd2b981029bf5ba9e1b9de3 (pr23095.05 -> pr23095.06, diff):

fixed fuzz test

DrahtBot added the label Needs rebase on Oct 21, 2021

refactor: Use C++17 std::clamp

This change improves code readability.

512dcf7d5b

hebasto force-pushed on Oct 27, 2021

hebasto commented at 6:14 AM on October 27, 2021: member

Updated 665353fa65da110a9bd2b981029bf5ba9e1b9de3 -> 512dcf7d5b7ff815ce4fec2d6fe27cbb13aa3e33 (pr23095.06 -> pr23095.07) due to the conflict with #23137.

DrahtBot removed the label Needs rebase on Oct 27, 2021

in src/miner.cpp:67 in 512dcf7d5b

  63 | @@ -64,7 +64,7 @@ BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool& mempoo
  64 |  {
  65 |      blockMinFeeRate = options.blockMinFeeRate;
  66 |      // Limit weight to between 4K and MAX_BLOCK_WEIGHT-4K for sanity:
  67 | -    nBlockMaxWeight = std::max<size_t>(4000, std::min<size_t>(MAX_BLOCK_WEIGHT - 4000, options.nBlockMaxWeight));
  68 | +    nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, 4000, MAX_BLOCK_WEIGHT - 4000);

laanwj commented at 1:36 PM on November 9, 2021:

Can we please add a static assert that MAX_BLOCK_WEIGHT >= 8000?

MarcoFalke commented at 1:50 PM on November 9, 2021:

Wouldn't it be better to have a clamp function that does this by default, internally and at compile time? The condition must hold for all call sites.

laanwj commented at 11:59 AM on November 17, 2021:

Though in most cases it's not possible to verify it at compile time, this would handle only the 'between two constexpr' case. In other cases there might be non-trivial error handling.

MarcoFalke commented at 12:03 PM on November 17, 2021:

For the cases where it is not possible to verify at compile time, crashing the node seems preferable over UB. Or is this something that sanitizers can spot?

laanwj commented at 1:38 PM on November 9, 2021: member

Code review ACK 512dcf7d5b7ff815ce4fec2d6fe27cbb13aa3e33 I checked all the changes and they look correct to me.

MarcoFalke commented at 1:48 PM on November 9, 2021: member

Slightly tend toward NACK. It is non-trivial to review (https://github.com/bitcoin/bitcoin/pull/23095#issuecomment-927779377) and easily introduces UB, if the second or third argument are not compile time constants (https://github.com/bitcoin/bitcoin/pull/23095#issuecomment-927780250).

hebasto commented at 7:49 PM on November 21, 2021: member

It is non-trivial to review (#23095 (comment)) and easily introduces UB.

Closing.

hebasto closed this on Nov 21, 2021

MarcoFalke commented at 10:09 AM on November 22, 2021: member

Maybe write a Clamp<int64_t, /*min=*/-1, /*max=*/3>(var); wrapper that has static asserts built in, and another one Clamp that copies std::clamp's signature that asserts/throws/returns optional on UB?

DrahtBot locked this on Nov 22, 2022