Related to #28280 (review), the CI build had a Sonar warning, namely:
“std::move” should not be called on an object of a type with neither move constructor nor move-assignment operator.
Added defaulted move constructor and move assignment operator to CCoinsCacheEntry.
Based on comments I’ve also added explicit special member functions to Coin.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30643.
See the guideline for information on the review process. A summary of reviews will appear here.
No conflicts as of last run.
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).
0 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:
(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:
0class Coin
1{
2 ...
3 Coin(Coin&&) noexcept = default;
4 ...
5};
configure clang-tidy to produce this warning in our own CI instead of Sonar
The [“coverage report”]( #30643 (comment)) 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
Coinas 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.
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.
42@@ -43,7 +43,12 @@ class Coin
43
44 //! construct a Coin from a CTxOut and height/coinbase information.
45 Coin(CTxOut&& outIn, int nHeightIn, bool fCoinBaseIn) : out(std::move(outIn)), fCoinBase(fCoinBaseIn), nHeight(nHeightIn) {}
46- Coin(const CTxOut& outIn, int nHeightIn, bool fCoinBaseIn) : out(outIn), fCoinBase(fCoinBaseIn),nHeight(nHeightIn) {}
47+ Coin(const CTxOut& outIn, int nHeightIn, bool fCoinBaseIn) : out(outIn), fCoinBase(fCoinBaseIn), nHeight(nHeightIn) {}
48+
49+ Coin(Coin&& other) noexcept = default;
50+ Coin& operator=(Coin&&) = default;
noexcept?
47+ Coin(const CTxOut& outIn, int nHeightIn, bool fCoinBaseIn) : out(outIn), fCoinBase(fCoinBaseIn), nHeight(nHeightIn) {}
48+
49+ Coin(Coin&& other) noexcept = default;
50+ Coin& operator=(Coin&&) noexcept = default;
51+ Coin(const Coin&) noexcept = default;
52+ Coin& operator=(const Coin&) noexcept = default;
Coin’s members’ respective default constructors that get invoked are noexcept?
According to https://en.cppreference.com/w/cpp/language/noexcept_spec:
default constructors, copy constructors, move constructors that are implicitly-declared or defaulted on their first declaration unless
- a constructor for a base or member that the implicit definition of the constructor would call is potentially-throwing (see below)
- a subexpression of such an initialization, such as a default argument expression, is potentially-throwing (see below)
- a default member initializer (for default constructor only) is potentially-throwing (see below)
So these noexcept specifiers here aren’t technically necessary. Doesn’t hurt to be explicit though, IMO.
157@@ -153,6 +158,13 @@ struct CCoinsCacheEntry
158 };
159
160 CCoinsCacheEntry() noexcept = default;
161+
162+ // No implicit copying, only moves.
163+ CCoinsCacheEntry(CCoinsCacheEntry&&) noexcept = default;
164+ CCoinsCacheEntry& operator=(CCoinsCacheEntry&&) noexcept = default;
165+ CCoinsCacheEntry(const CCoinsCacheEntry&) noexcept = delete;
166+ CCoinsCacheEntry& operator=(const CCoinsCacheEntry&) noexcept = delete;
noexcept
Am I right in understanding that this PR’s expected changes are:
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
CCoinsCacheEntryreference 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
