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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30643.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

Don't we also need to add move constructor to Coin? Its prevector is the only thing that really benefits from being moved I believe.

Sonar

They are mostly just a clang-tidy wrapper, so I wonder if there is a clang-tidy warning that can be used here instead.

I wonder if there is a clang-tidy warning

I have installed Sonar locally, reproduced the warning first, it doesn't appear anymore afer the change.

Don't we also need to add move constructor to Coin?

It seems to me that the Coin class already has an explicit move constructor that moves the CTxOut member - and the rest are trivially movable (i.e. bit-copy of unsigned int and uint32_t).

    Coin(CTxOut&& outIn, int nHeightIn, bool fCoinBaseIn) : out(std::move(outIn)), fCoinBase(fCoinBaseIn), nHeight(nHeightIn) {}

The CTxOut class itself does not have an explicitly defined move constructor or move assignment operator, though it can rely on the compiler-generated defaults (since CAmount = int64_t, which is trivially movable). CScript itself does not explicitly declare any additional members beyond what it inherits from CScriptBase (and prevector does have move a constructor), so I think the compiler can automatically generate a default move constructor for CScript.

If you suspect I'm wrong here, let me know where you'd like me to investigate further.

reproduded the warning first, it doesn't appear anymore afer the change.

Can you add any relevant output to the PR description.

I wonder if there is a clang-tidy warning that can be used here instead.

I have installed Sonar locally, reproduced the warning first, it doesn't appear anymore afer the change.

I think what is being suggested here is to configure clang-tidy to produce this warning in our own CI instead of Sonar, which can then be shown to be fixed in this PR. I think that would be a more valuable change.

It seems to me that the Coin class already has an explicit move constructor that moves the CTxOut member

That would not be an explicit move constructor, right? That is a constructor that has one of its members moved in. An explicit move constructor would be Coin(Coin&& coin).

so I think the compiler can automatically generate a default move constructor for CScript.

How can we be sure that this happens?

How can we be sure that this happens?

A class has an implicitly-declared move constructor whenever:

• There are no user-declared copy constructors.
• There are no user-declared copy assignment operators.
• There are no user-declared move constructors.
• There is no user-declared destructor

(from https://en.cppreference.com/w/cpp/language/move_constructor)

Personally, I find these rules too inscrutable to judge from a quick glance at a class, so if I want a class that is guaranteed to have a move constructor, I just add one explicitly:

class Coin
{
    ...
    Coin(Coin&&) noexcept = default;
    ...
};

configure clang-tidy to produce this warning in our own CI instead of Sonar

The "coverage report" already has the Sonarcloud section - I have opened this issue because of the #28280 corecheck CI Sonar warnings. Or do you mean we should be able to reproduce this locally as well via clang-tidy?

if I want a class that is guaranteed to have a move constructor, I just add one explicitly

Done for Coin as well, thanks.

configure clang-tidy to produce this warning in our own CI instead of Sonar

The "coverage report" already has the Sonarcloud section - I have opened this issue because of the #28280 corecheck CI Sonar warnings. Or do you mean we should be able to reproduce this locally as well via clang-tidy?

if I want a class that is guaranteed to have a move constructor, I just add one explicitly

Done dor Coin as well, thanks.

For testing locally, one can add a static assert, similar to src/validation.h:static_assert(std::is_nothrow_move_constructible_v<CScriptCheck>);. (This is also an alternative to adding move constructors explicitly).

I was mostly wondering about other cases, which would be nice to be able to find by clang-tidy, but that is probably only tangentially related to this pull.

@maflcko etd:is_nothrow_move_constructible_v will also return true if a nothrow copy constructor is available (because a copy constructor is eligible in any context where a move constructor is called), so that isn't enough to verify that an efficient version exists.

cc @davidgumberg @andrewtoth

Am I right in understanding that this PR's expected changes are:

1. Deletion of the CCoinsCacheEntry reference copy constructors
2. Update CoinsMapEntry in the coins unit tests to try_emplace over emplace.

And we are otherwise just explicitly declaring the move constructors that we expect to already be used implicitly in order to silence a spurious CI warning?

Also, could you point me in the direction of reproducing this CI warning locally? I can't find the warning on corecheck (https://corecheck.dev/bitcoin/bitcoin/pulls/28280)

Thanks for checking @davidgumberg!

Also, could you point me in the direction of reproducing this CI warning locally? I can't find the warning on corecheck (corecheck.dev/bitcoin/bitcoin/pulls/28280)

I also had a hard time reproducing it on CI (sonarcould seems to have expired), but installed Sonar locally which gave the same warning before the change.
But the before state is not as important as the after state: we need to move Coin and CCoinsCacheEntry so an explicit constructor is better.

Deletion of the CCoinsCacheEntry reference copy constructors

This is just a safety measure

explicitly declaring the move constructors that we expect to already be used implicitly

Either that or adding a move constructor, since it's needed (regardless of whether it was there implicitly or not)

Update CoinsMapEntry in the coins unit tests to try_emplaceoveremplace`

This was added to have an actual user of the (new?) move constructor