The Clang tidy CI passed, so opening this up for review.

The change on line 52 makes sense, but the one on 316 seems like a step backwards?

The change on line 52 makes sense, but the one on 316 seems like a step backwards?

Hm, RVO wouldn't be affected by the move of an argument (rather than of the object itself)?

Repushed without that change to see if the tidy job remains appeased.

Hm, RVO wouldn't be affected by the move of an argument (rather than of the object itself)?

I don't see how RVO could apply here. It's an argument to a constructor. The fact that that constructor is invoked in a return statement doesn't matter.

@jonatack Oh! The issue is just that CBlockLocator has no constructor that accepts an rvalue uint256. The std::move there has no effect, but the correct solution is adding an rvalue-accepting constructor, not getting rid of the std::move.

@jonatack Oh! The issue is just that CBlockLocator has no constructor that accepts an rvalue uint256. The std::move there has no effect, but the correct solution is adding an rvalue-accepting constructor, not getting rid of the std::move.

Ah interesting! Learning.

ACK af7a40c
(the previous push with the two changes)

Both warnings are legit because no move will happen in either case.

But consider this, like @sipa suggested above, would avoid copying:

diff --git i/src/chain.cpp w/src/chain.cpp
index 19c35b5012..66a0830394 100644
--- i/src/chain.cpp
+++ w/src/chain.cpp
@@ -46,13 +46,13 @@ std::vector<uint256> LocatorEntries(const CBlockIndex* index)
     }
     return have;
 }

 CBlockLocator GetLocator(const CBlockIndex* index)
 {
-    return CBlockLocator{std::move(LocatorEntries(index))};
+    return CBlockLocator{LocatorEntries(index)};
 }

 CBlockLocator CChain::GetLocator() const
 {
     return ::GetLocator(Tip());
 }
diff --git i/src/primitives/block.h w/src/primitives/block.h
index 76aba6c899..8c51d280b8 100644
--- i/src/primitives/block.h
+++ w/src/primitives/block.h
@@ -120,13 +120,13 @@ public:
 struct CBlockLocator
 {
     std::vector<uint256> vHave;

     CBlockLocator() {}

-    explicit CBlockLocator(const std::vector<uint256>& vHaveIn) : vHave(vHaveIn) {}
+    explicit CBlockLocator(std::vector<uint256>&& vHaveIn) : vHave(std::move(vHaveIn)) {}

     SERIALIZE_METHODS(CBlockLocator, obj)
     {
         int nVersion = s.GetVersion();
         if (!(s.GetType() & SER_GETHASH))
             READWRITE(nVersion);

Using && would force all call sites to use std::move. Thus, it makes it impossible to use on const&.

You can simply change the constructor to allow anything:

explicit CBlockLocator(std::vector<uint256> have) : vHave{std::move(have)} {}

@MarcoFalke I think all call sites already pass in rvalues. And an rvalue argument is still more efficient than a by-value one, as it avoids constructing the temporary vector entirely (not just avoiding allocating its storage buffer).

an rvalue argument is still more efficient than a by-value one

Correct. Personally, I think it is fine to have an overhead equivalent to constructing an empty vector. When there is a new call-site with a const reference in the future, the constructor will need to be changed anyway. Either a single one that accepts a pure value, or a new one that accepts a const& (alongside the && constructor), which is what the stdlib is usually doing.

But anything should be fine here.

review ACK 6b24dfe

With a by-value argument there is construct+move+move, with an rval argument, there is construct+move:

#include <iostream>

/* The rule of five. */
class R5
{
public:
    R5(int x, double y)
    {
        x_ = x;
        y_ = y;
        std::cout << "R5::R5() this=" << this << " x=" << x_ << ", y=" << y_ << std::endl;
    }

    ~R5()
    {
        std::cout << "R5::~R5() this=" << this << " x=" << x_ << ", y=" << y_ << std::endl;
    }

    R5(const R5& other)
    {
        std::cout << "R5::R5(const R5&) this=" << this << " other=" << &other << " x=" << other.x_
                  << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
    }

    R5(R5&& other)
    {
        std::cout << "R5::R5(R5&&) this=" << this << " other=" << &other << " x=" << other.x_
                  << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
        other.x_ = 0;
        other.y_ = 0;
    }

    R5& operator=(const R5& other)
    {
        std::cout << "R5::operator=(const R5&) this=" << this << " x=" << x_ << ", y=" << y_
                  << " other=" << &other << " x=" << other.x_ << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
        return *this;
    }

    R5& operator=(R5&& other)
    {
        std::cout << "R5::operator=(R5&&) this=" << this << " x=" << x_ << ", y=" << y_
                  << " other=" << &other << " x=" << other.x_ << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
        other.x_ = 0;
        other.y_ = 0;
        return *this;
    }

    std::ostream& operator<<(std::ostream& os) { return os << "(x=" << x_ << ", y=" << y_ << ")"; }

    int x_;
    double y_;
};

class CBlockLocator
{
public:
#if 0
    explicit CBlockLocator(R5 r5) : m_r5{std::move(r5)} {}
#else
    explicit CBlockLocator(R5&& r5) : m_r5{std::move(r5)} {}
#endif

    R5 m_r5;
};

CBlockLocator f3()
{
    R5 r5{2, 3.4};
    return CBlockLocator{std::move(r5)};
}

int main(int, char**)
{
    f3();
    return 0;
}

output with R5 r5 arg:

R5::R5() this=0x7fffffffe5e8 x=2, y=3.4
R5::R5(R5&&) this=0x7fffffffe5d8 other=0x7fffffffe5e8 x=2, y=3.4
R5::R5(R5&&) this=0x7fffffffe610 other=0x7fffffffe5d8 x=2, y=3.4
R5::~R5() this=0x7fffffffe5d8 x=0, y=0
R5::~R5() this=0x7fffffffe5e8 x=0, y=0
R5::~R5() this=0x7fffffffe610 x=2, y=3.4

output with R5&& r5 arg:

R5::R5() this=0x7fffffffe5e8 x=2, y=3.4
R5::R5(R5&&) this=0x7fffffffe610 other=0x7fffffffe5e8 x=2, y=3.4
R5::~R5() this=0x7fffffffe5e8 x=0, y=0
R5::~R5() this=0x7fffffffe610 x=2, y=3.4

Correct. Personally, I think it is fine to have an overhead equivalent to constructing an empty vector. When there is a new call-site with a const reference in the future, the constructor will need to be changed anyway. Either a single one that accepts a pure value, or a new one that accepts a const& (alongside the && constructor), which is what the stdlib is usually doing.

Yes, to be clear: none of the changes in this PR are relevant for performance. CBlockLocators are constructed sufficiently rarely that shaving off (maybe) a microsecond here and there isn't going to be measurable. But if we're already talking about changing the code anyway, it's good to follow best practices.

Going to merge now to unbreak CI. If there is anything else to be done here, it can be done in a follow-up.

With a by-value argument there is construct+move+move, with an rval argument, there is construct+move:
rv.cc

output with `R5 r5` arg:

R5::R5() this=0x7fffffffe5e8 x=2, y=3.4
R5::R5(R5&&) this=0x7fffffffe5d8 other=0x7fffffffe5e8 x=2, y=3.4
R5::R5(R5&&) this=0x7fffffffe610 other=0x7fffffffe5d8 x=2, y=3.4
R5::~R5() this=0x7fffffffe5d8 x=0, y=0
R5::~R5() this=0x7fffffffe5e8 x=0, y=0
R5::~R5() this=0x7fffffffe610 x=2, y=3.4

output with `R5&& r5` arg:

R5::R5() this=0x7fffffffe5e8 x=2, y=3.4
R5::R5(R5&&) this=0x7fffffffe610 other=0x7fffffffe5e8 x=2, y=3.4
R5::~R5() this=0x7fffffffe5e8 x=0, y=0
R5::~R5() this=0x7fffffffe610 x=2, y=3.4

@vasild nice! Reproduced this locally for fun.

₿ g++ by-val-versus-rval.cpp && ./a.out 
R5::R5() this=0x7ffe5de81ea0 x=2, y=3.4
R5::R5(R5&&) this=0x7ffe5de81eb0 other=0x7ffe5de81ea0 x=2, y=3.4
R5::R5(R5&&) this=0x7ffe5de81ef0 other=0x7ffe5de81eb0 x=2, y=3.4
R5::~R5() this=0x7ffe5de81eb0 x=0, y=0
R5::~R5() this=0x7ffe5de81ea0 x=0, y=0
R5::~R5() this=0x7ffe5de81ef0 x=2, y=3.4

₿ g++ by-val-versus-rval.cpp && ./a.out 
R5::R5() this=0x7ffea6450b90 x=2, y=3.4
R5::R5(R5&&) this=0x7ffea6450bd0 other=0x7ffea6450b90 x=2, y=3.4
R5::~R5() this=0x7ffea6450b90 x=0, y=0
R5::~R5() this=0x7ffea6450bd0 x=2, y=3.4