refactor: Use C++17 std::clamp by hebasto · Pull Request #23095 · bitcoin/bitcoin

hebasto · 2021-09-25T12:11:24Z

This PR improves code readability.

Only safe cases with guarantee of no undefined behavior.

DrahtBot · 2021-09-25T14:40:35Z

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.

theStack · 2021-09-26T21:03:08Z

Concept ACK

jarolrod · 2021-09-27T02:38:05Z

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 · 2021-09-27T07:43:10Z

Concept ACK

I wonder whether this is a good candidate or not:

bitcoin/src/rpc/blockchain.cpp

Line 1859 in 58c25bd

blockcount = std::max(0, std::min(blockcount, pindex->nHeight - 1));

hebasto · 2021-09-27T08:24:40Z

@kiminuo

I wonder whether this is a good candidate or not:

bitcoin/src/rpc/blockchain.cpp

Line 1859 in 58c25bd

blockcount = std::max(0, std::min(blockcount, pindex->nHeight - 1));

Some tests fail with this line being changed.

kiminuo · 2021-09-27T08:34:56Z

@kiminuo

I wonder whether this is a good candidate or not:

bitcoin/src/rpc/blockchain.cpp

Line 1859 in 58c25bd

blockcount = std::max(0, std::min(blockcount, pindex->nHeight - 1));

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 · 2021-09-27T08:43:23Z

@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: 7583dee
CI tests: https://cirrus-ci.com/build/4594368825786368

laanwj · 2021-09-27T11:11:05Z

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.

maflcko · 2021-09-27T11:22:50Z

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 · 2021-09-27T11:23:28Z

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 :)

src/checkqueue.h

src/init.cpp

hebasto · 2021-09-28T07:54:39Z

Updated 82c4e71 -> bf4838e (pr23095.04 -> pr23095.05):

rebased
addressed comments

martinus

Code review ACK bf4838e

hebasto · 2021-09-29T08:24:33Z

Updated bf4838e -> 665353f (pr23095.05 -> pr23095.06, diff):

fixed fuzz test

This change improves code readability.

hebasto · 2021-10-27T06:14:03Z

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

laanwj · 2021-11-09T13:36:53Z

src/miner.cpp

    blockMinFeeRate = options.blockMinFeeRate;
    // Limit weight to between 4K and MAX_BLOCK_WEIGHT-4K for sanity:
-    nBlockMaxWeight = std::max<size_t>(4000, std::min<size_t>(MAX_BLOCK_WEIGHT - 4000, options.nBlockMaxWeight));
+    nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, 4000, MAX_BLOCK_WEIGHT - 4000);


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

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.

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.

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 · 2021-11-09T13:38:42Z

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

maflcko · 2021-11-09T13:48:53Z

Slightly tend toward NACK. It is non-trivial to review (#23095 (comment)) and easily introduces UB, if the second or third argument are not compile time constants (#23095 (comment)).

hebasto · 2021-11-21T19:49:01Z

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

Closing.

maflcko · 2021-11-22T10:09:54Z

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?

hebasto added the Refactoring label Sep 25, 2021

hebasto force-pushed the 210925-clamp branch from 7583dee to 334df35 Compare September 25, 2021 15:01

kiminuo mentioned this pull request Sep 25, 2021

scripted-diff: Rename overloaded int GetArg to GetIntArg #22976

Merged

hebasto force-pushed the 210925-clamp branch from 334df35 to d335443 Compare September 25, 2021 18:47

hebasto marked this pull request as draft September 25, 2021 19:54

hebasto force-pushed the 210925-clamp branch from d335443 to 82c4e71 Compare September 25, 2021 21:32

DrahtBot mentioned this pull request Sep 26, 2021

Improve Indices on pruned nodes via prune blockers #21726

Merged

hebasto marked this pull request as ready for review September 26, 2021 05:12

martinus suggested changes Sep 27, 2021

View reviewed changes

src/checkqueue.h Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Sep 27, 2021

hebasto force-pushed the 210925-clamp branch from 82c4e71 to bf4838e Compare September 28, 2021 07:51

martinus approved these changes Sep 28, 2021

View reviewed changes

DrahtBot removed the Needs rebase label Sep 28, 2021

hebasto force-pushed the 210925-clamp branch from bf4838e to 665353f Compare September 28, 2021 10:33

This was referenced Oct 8, 2021

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

Add fee_est tool for debugging fee estimation code #10443

Closed

DrahtBot added the Needs rebase label Oct 21, 2021

refactor: Use C++17 std::clamp

512dcf7

This change improves code readability.

hebasto force-pushed the 210925-clamp branch from 665353f to 512dcf7 Compare October 27, 2021 06:12

DrahtBot removed the Needs rebase label Oct 27, 2021

laanwj reviewed Nov 9, 2021

View reviewed changes

DrahtBot mentioned this pull request Nov 13, 2021

Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces #23497

Merged

hebasto closed this Nov 21, 2021

bitcoin locked and limited conversation to collaborators Nov 22, 2022

Conversation

hebasto commented Sep 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack commented Sep 26, 2021

Uh oh!

jarolrod commented Sep 27, 2021

Uh oh!

kiminuo commented Sep 27, 2021

Uh oh!

hebasto commented Sep 27, 2021

Uh oh!

kiminuo commented Sep 27, 2021

Uh oh!

hebasto commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 27, 2021

Uh oh!

maflcko commented Sep 27, 2021

Uh oh!

martinus commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented Sep 28, 2021

Uh oh!

martinus left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Sep 29, 2021

Uh oh!

hebasto commented Oct 27, 2021

Uh oh!

laanwj Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 9, 2021

Uh oh!

maflcko commented Nov 9, 2021

Uh oh!

hebasto commented Nov 21, 2021

Uh oh!

maflcko commented Nov 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hebasto commented Sep 25, 2021 •

edited

Loading

DrahtBot commented Sep 25, 2021 •

edited

Loading

hebasto commented Sep 27, 2021 •

edited

Loading

martinus commented Sep 27, 2021 •

edited

Loading

laanwj Nov 9, 2021 •

edited

Loading