refactor: C++20: Use std::rotl #29085

fjahr commented at 10:49 PM on December 14, 2023: contributor

While exploring some C++20 changes and checking against our code I found this potential improvement:

We can replace our custom implementation of rotl32 in crypto/chacha20 with std::rotl from the new bit header.

DrahtBot commented at 10:49 PM on December 14, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fanquake
Concept ACK	hebasto
Stale ACK	maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Refactoring on Dec 14, 2023

DrahtBot added the label CI failed on Dec 14, 2023

maflcko commented at 8:52 AM on December 15, 2023: member

Not sure. This needs a benchmark to check against slowdown, see #29057 (comment)

Also, C++20 is enabled for years, so I don't understand the other commit. I think you either forgot to compile locally, or your compiler doesn't emit any warnings that the changes are wrong?

fanquake commented at 9:53 AM on December 15, 2023: member

txmempool.cpp:88:61: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
   88 |             mapTx.modify(mapTx.iterator_to(descendant), [=, this](CTxMemPoolEntry& e) {
      |                                                           ~~^~~~
txmempool.cpp:99:32: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
   99 |     mapTx.modify(updateIt, [=, this](CTxMemPoolEntry& e) { e.UpdateDescendantState(modifySize, modifyFee, modifyCount); });
      |                              ~~^~~~
txmempool.cpp:300:38: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
  300 |         mapTx.modify(ancestorIt, [=, this](CTxMemPoolEntry& e) { e.UpdateDescendantState(updateSize, updateFee, updateCount); });
      |                                    ~~^~~~
txmempool.cpp:315:26: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
  315 |     mapTx.modify(it, [=, this](CTxMemPoolEntry& e){ e.UpdateAncestorState(updateSize, updateFee, updateCount, updateSigOpsCost); });
      |                        ~~^~~~
txmempool.cpp:345:39: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
  345 |                 mapTx.modify(dit, [=, this](CTxMemPoolEntry& e){ e.UpdateAncestorState(modifySize, modifyFee, -1, modifySigOps); });
      |                                     ~~^~~~
txmempool.cpp:903:46: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
  903 |                 mapTx.modify(ancestorIt, [=, this](CTxMemPoolEntry& e){ e.UpdateDescendantState(0, nFeeDelta, 0);});
      |                                            ~~^~~~
txmempool.cpp:910:48: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
  910 |                 mapTx.modify(descendantIt, [=, this](CTxMemPoolEntry& e){ e.UpdateAncestorState(0, nFeeDelta, 0, 0); });
      |                                              ~~^~~~

fjahr commented at 10:26 PM on December 15, 2023: contributor

Also, C++20 is enabled for years, so I don't understand the other commit.

I don't understand this comment. What's the 'other' commit?

I think you either forgot to compile locally, or your compiler doesn't emit any warnings that the changes are wrong?

I compiled locally just fine and ran all test using Apple clang version 15.0.0 as the compiler. I will look into the warnings.

mzumsande commented at 10:45 PM on December 15, 2023: contributor

I think the issue is that most of the spots that were changed don't actually make use of the implicit capture of this (e.g. by accessing a class or struct member in the body of the lambda) and therefore don't need to be changed. Only the example in the scheduler does that (which is a doc). Not the use of [=] in general is deprecated, just using [=] in combination with accessing this via implicit capture. See https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0806r2.html for a longer explanation.

fjahr force-pushed on Dec 15, 2023

fjahr commented at 11:18 PM on December 15, 2023: contributor

Seems right that the capture doesn't always need to be explicit, I dropped the second commit.

I don't see any performance impact on chacha20 from the first commit:

<details> <summary>MASTER</summary>