refactor: remove unnecessary std::move for trivially copyable types

l0rinc commented at 1:27 pm on February 5, 2026: contributor

Problem

A few function signatures in the codebase use rvalue references for trivially copyable types, forcing callers to std::move() values where there is no benefit - for trivially copyable types, moving is semantically identical to copying (see https://godbolt.org/z/hdrn17v9b).

Additionally, some call sites use std::move() on plain enums and primitive types where it is basically just noise.

Note: CheckTriviallyCopyableMove remains false - std::move() on trivially copyable types is still permitted where it serves as intent documentation (e.g. signaling that a value should not be reused after a call).

Fix

Document why CheckTriviallyCopyableMove is kept disabled (to preserve bugprone-use-after-move coverage on trivially copyable types where std::move signals intent), cherry-picked from #34523
Change EmplaceCoinInternalDANGER to take const COutPoint& instead of COutPoint&&
Change logging functions to take const SourceLocation& instead of SourceLocation&&
Remove std::move() on enum types in RPC constructors and on bool/int members in txgraph.cpp

DrahtBot commented at 1:28 pm on February 5, 2026: contributor

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

Reviews

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33512 (coins: use number of dirty cache entries in flush warnings/logs by l0rinc)

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.

fanquake renamed this:
~~L0rinc/move const arg trivcopy~~
refactor: enable `move-const-arg` for trivially-copyable types
on Feb 5, 2026

DrahtBot added the label Refactoring on Feb 5, 2026

in src/txmempool.cpp:177 in 32f00a544d

176 }
177 
178-CTxMemPool::CTxMemPool(Options opts, bilingual_str& error)
179-    : m_opts{Flatten(std::move(opts), error)}
180+CTxMemPool::CTxMemPool(const Options& opts, bilingual_str& error)
181+    : m_opts{Flatten(opts, error)}

maflcko commented at 2:26 pm on February 5, 2026:

This was written intentionally, because:

If the options layout changes in the future, it will optimize “for free”
There is no harm in having the move, other than it being a bit confusing
Using std::move also has the benefit to mark a symbol used, regardless of its memory layout (ref use-after-move clang-tidy check)

andrewtoth commented at 2:59 pm on February 5, 2026:

I’m not sure about the third point.

the benefit to mark a symbol as used

Why is it a benefit in this case? Wouldn’t this be better leaving the constructor parameter alone and passing a non-const ref to Flatten?

maflcko commented at 4:27 pm on February 5, 2026:

right. Maybe here, but not in src/random.cpp. Re-using a cleared vanilla hasher after move for randomness seems slightly non-ideal.

l0rinc commented at 10:22 am on February 6, 2026:

Re-using a cleared vanilla hasher after move for randomness seems slightly non-ideal

I also found that confusing, reverted it here.

If the options layout changes in the future, it will optimize “for free”

I don’t agree with this, but I have reverted it, we can discuss it separately.

maflcko commented at 2:27 pm on February 5, 2026: member

Not sure, but I don’t mind the change.

Just to clarify, this was done intentionally, see the reasons inline. Maybe docs or a comment could be added to clarify this?

in src/logging.h:1 in a6f725e904 outdated

hodlinator commented at 2:54 pm on February 5, 2026:

nit: It is unspecified whether std::source_location is trivial or not: https://en.cppreference.com/w/cpp/utility/source_location/source_location.html

l0rinc commented at 10:22 am on February 6, 2026:

Thanks, added a static_assert in logging_LogPrintStr for SourceLocation triviality to document why we’re not using std::move for these types

in src/validation.cpp:1851 in 32f00a544d

1848@@ -1849,7 +1849,7 @@ CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams)
1849 }
1850 
1851 CoinsViews::CoinsViews(DBParams db_params, CoinsViewOptions options)

hodlinator commented at 3:02 pm on February 5, 2026:

nit: Might as well dodge one copy, even though the type is barely bigger than a pointer:

0CoinsViews::CoinsViews(DBParams db_params, const CoinsViewOptions& options)

purpleKarrot commented at 3:28 pm on February 5, 2026:

In constructors, the pass-by-value-and-move-into-place is the right approach. This only applies to constructors, not setter functions. See https://stackoverflow.com/questions/26261007/why-is-value-taking-setter-member-functions-not-recommended-in-herb-sutters-cpp for details.

hodlinator commented at 7:28 pm on February 5, 2026:

That makes sense for types like std::string which likely contain pointers to memory in other locations (also true for DBParams in this example). It’s not as clear-cut for simpler types like CoinsViewOptions: https://github.com/bitcoin/bitcoin/blob/32f00a544d3c0adb066c6ffb375787be9bb8c6cb/src/txdb.h#L26-L31

Taking by const-ref should be slightly more optimal, but it’s not important to me. Maybe the 2 fields of the struct can be passed in 2 registers, I’m not too familiar with calling conventions.

l0rinc commented at 10:22 am on February 6, 2026:

Reverted all Option moves

purpleKarrot commented at 3:50 pm on February 5, 2026: contributor

Using std::move on a const-qualified object has no observable effect in code generation, and it harms readability. The reader may assume that it causes a movable type to move, when it is actually copied.

Using std::move on trivially copyable types also has no observable effect in code generation, but it may improve readability to use it, because it signals intent, as @maflcko wrote above (“mark a symbol used”).

I would consider setting CheckTriviallyCopyableMove to false in move-const-arg. Also, make sure not to introduce regressions for pass-by-value.

maflcko commented at 3:54 pm on February 5, 2026: member

Using std::move on a const-qualified object has no observable effect in code generation, and it harms readability. The reader may assume that it causes a movable type to move, when it is actually copied.

Using std::move on trivially copyable types also has no observable effect in code generation, but it may improve readability to use it, because it signals intent, as @maflcko wrote above (“mark a symbol used”).

I would consider setting CheckTriviallyCopyableMove to false in move-const-arg. Also, make sure not to introduce regressions for pass-by-value.

I think all of your points are already implemented since faad673716c (zirka 2022)

hodlinator commented at 7:32 pm on February 5, 2026: contributor

I’m ~0 on this:

It’s nice to clear away what can be seen as noise.
It’s nice to document intent (“mark a symbol used”) as 2 prior commenters have pointed out.

maflcko commented at 7:50 am on February 6, 2026: member

Fixed the docs in https://github.com/bitcoin/bitcoin/pull/34523

l0rinc force-pushed on Feb 6, 2026

l0rinc commented at 10:25 am on February 6, 2026: contributor

Thanks for the comments, while I think signaling that a value should not be reused after a call via std::move is kinda’ hacky and arbitrary, I have dropped the CSHA512 signature change in random.cpp, the CTxMemPool::Options, cherry-picked the CheckTriviallyCopyableMove move changes and most mechanical clang-tidy fix-its. Let me know what you think, I hope I have addressed all concerns.

l0rinc renamed this:
~~refactor: enable `move-const-arg` for trivially-copyable types~~
refactor: remove unnecessary std::move for trivially copyable types
on Feb 6, 2026

l0rinc requested review from purpleKarrot on Feb 6, 2026

l0rinc requested review from andrewtoth on Feb 6, 2026

l0rinc requested review from maflcko on Feb 6, 2026

l0rinc requested review from hodlinator on Feb 6, 2026

maflcko commented at 1:56 pm on February 6, 2026: member

lgtm, but there are a few conflicts let’s get those in earlier.

Feel free to ping me for review after a week of inactivity or so, but i don’t think there is need to ping reviewers on the second day of a refactor pull request.

DrahtBot added the label Needs rebase on Feb 7, 2026

l0rinc force-pushed on Feb 8, 2026

bvbfan commented at 8:08 am on February 8, 2026: contributor

Why not introduce function move_if_not_trivially_copyable so you will have docs inside the code.

DrahtBot removed the label Needs rebase on Feb 8, 2026

fanquake referenced this in commit 6ca7782db9 on Feb 9, 2026

coins: avoid moving `COutPoint` in snapshot load

`EmplaceCoinInternalDANGER` took `COutPoint&&`, forcing callers to `std::move()` a trivially-copyable value.

Take the `outpoint` by const reference instead.

Co-authored-by: Andrew Toth <andrewstoth@gmail.com>

b2d234a12b

logging: pass `SourceLocation` by const reference

Logging helpers took `SourceLocation&&`, forcing pointless `std::move()` at call sites even though `SourceLocation` is trivially copyable.
Added static assert to related test to document this being the case.

Accept `const SourceLocation&` in `LogPrintFormatInternal`, `LogPrintStr`, and `LogPrintStr_`.

f5913d43f1

rpc: remove `std::move` on `RPCArg::Type` and `RPCResult::Type` enums in constructors 20d3b5a391

txgraph: remove unnecessary `std::move` on primitive members 1414bfeab9

l0rinc force-pushed on Feb 9, 2026

l0rinc commented at 2:03 pm on February 9, 2026: contributor

there are a few conflicts let’s get those in earlier

Rebased after #34523, the conflict list only contains my own change now.

Why not introduce function move_if_not_trivially_copyable so you will have docs inside the code.

You mean like the https://en.cppreference.com/w/cpp/utility/move_if_noexcept.html, but for the trivial types that we still want to avoid reusing? I’m not sure about that, but maybe if you could give a concrete diff it would help.

bvbfan commented at 6:23 pm on February 9, 2026: contributor

Why not introduce function move_if_not_trivially_copyable so you will have docs inside the code.

You mean like the https://en.cppreference.com/w/cpp/utility/move_if_noexcept.html, but for the trivial types that we still want to avoid reusing? I’m not sure about that, but maybe if you could give a concrete diff it would help.

Yep, exactly like that

0template<typename T>
1std::conditional_t<std::is_trivially_copyable_v<T>, T, T&&>
2move_if_not_trivialy_copyable(T& x) {
3    if constexpr (std::is_trivially_copyable_v<T>) {
4        return x;
5    }
6    return std::move(x);
7}

maflcko commented at 9:13 am on February 10, 2026: member

I think it would be easier to enforce this with clang-tidy via:

Also, make sure not to introduce regressions for pass-by-value.

instead of move_if_not_trivialy_copyable.

However, the storage of ipv6 in CNetAddr::m_addr is meant to be trivially copyable. However prevector isn’t, even if only used with direct storage. So it could make sense to fix that first, by introducing a (let’s say) ArrayVec, which is backed by a fixed-size array and a variable size counter (up to the fixed-size array len)

l0rinc commented at 10:18 am on February 10, 2026: contributor

So it could make sense to fix that first

CNetAddr::m_addr/prevector sounds like a larger design cleanup, I don’t mind doing that in a separate PR.

make sure not to introduce regressions for pass-by-value.

running it locally and grepping for [modernize-pass-by-value] shows the exact same list before and after (cc: @purpleKarrot):

 0src/httpserver.cpp:131:21: warning: pass by value and use std::move [modernize-pass-by-value]
 1src/httpserver.cpp:131:60: warning: pass by value and use std::move [modernize-pass-by-value]
 2src/i2p.cpp:122:18: warning: pass by value and use std::move [modernize-pass-by-value]
 3src/i2p.cpp:130:45: warning: pass by value and use std::move [modernize-pass-by-value]
 4src/net.cpp:3374:20: warning: pass by value and use std::move [modernize-pass-by-value]
 5src/net.cpp:3968:14: warning: pass by value and use std::move [modernize-pass-by-value]
 6src/test/bip32_tests.cpp:29:25: warning: pass by value and use std::move [modernize-pass-by-value]
 7src/test/blockfilter_index_tests.cpp:317:72: warning: pass by value and use std::move [modernize-pass-by-value]
 8src/test/util/net.cpp:341:18: warning: pass by value and use std::move [modernize-pass-by-value]
 9src/test/util/net.cpp:341:48: warning: pass by value and use std::move [modernize-pass-by-value]
10src/wallet/test/db_tests.cpp:218:19: warning: pass by value and use std::move [modernize-pass-by-value]

maflcko commented at 4:59 pm on February 10, 2026: member

CNetAddr::m_addr/prevector sounds like a larger design cleanup, I don’t mind doing that in a separate PR.

Yeah, right. Looks a bit more verbose:

  0diff --git a/src/netaddress.cpp b/src/netaddress.cpp
  1index fb2c254076..201c228283 100644
  2--- a/src/netaddress.cpp
  3+++ b/src/netaddress.cpp
  4@@ -26,6 +26,8 @@ using util::HasPrefix;
  5 
  6 CNetAddr::BIP155Network CNetAddr::GetBIP155Network() const
  7 {
  8+    static_assert(std::is_trivially_copyable_v<CNetAddr>);
  9+    static_assert(std::is_trivially_copyable_v<decltype(CNetAddr::m_addr)>);
 10     switch (m_net) {
 11     case NET_IPV4:
 12         return BIP155Network::IPV4;
 13diff --git a/src/netaddress.h b/src/netaddress.h
 14index 2191da54b7..86463879b1 100644
 15--- a/src/netaddress.h
 16+++ b/src/netaddress.h
 17@@ -106,6 +106,93 @@ static constexpr uint16_t I2P_SAM31_PORT{0};
 18 
 19 std::string OnionToString(std::span<const uint8_t> addr);
 20 
 21+#include <algorithm>
 22+#include <cstddef>
 23+#include <stdexcept>
 24+
 25+template <std::size_t N, BasicByte B>
 26+class ByteArrayVec
 27+{
 28+private:
 29+    B data_[N]{};
 30+    std::size_t size_{0};
 31+
 32+public:
 33+    constexpr ByteArrayVec() = default;
 34+
 35+    constexpr void push_back(B b)
 36+    {
 37+        if (size_ >= N) [[unlikely]] {
 38+            throw std::out_of_range("ByteArrayVec capacity exceeded");
 39+        }
 40+        data_[size_++] = b;
 41+    }
 42+
 43+    constexpr void pop_back()
 44+    {
 45+        if (size_ == 0) [[unlikely]] {
 46+            throw std::out_of_range("ByteArrayVec underflow: pop_back called on empty container");
 47+        }
 48+        --size_;
 49+    }
 50+
 51+    constexpr void resize(std::size_t new_size)
 52+    {
 53+        if (new_size > N) [[unlikely]] {
 54+            throw std::out_of_range("ByteArrayVec resize exceeds capacity");
 55+        }
 56+        if (new_size > size_) {
 57+            // Zero-initialize the appended bytes
 58+            std::fill_n(data_ + size_, new_size - size_, B{0});
 59+        }
 60+        size_ = new_size;
 61+    }
 62+
 63+    constexpr void assign(std::size_t count, B value)
 64+    {
 65+        if (count > N) [[unlikely]] {
 66+            throw std::out_of_range("ByteArrayVec assign exceeds capacity");
 67+        }
 68+        std::fill_n(data_, count, value);
 69+        size_ = count;
 70+    }
 71+
 72+    template <std::input_iterator It>
 73+    constexpr void assign(It first, It last)
 74+    {
 75+        const auto new_size = static_cast<std::size_t>(std::distance(first, last));
 76+        if (new_size > N) [[unlikely]] {
 77+            throw std::out_of_range("ByteArrayVec assign range exceeds capacity");
 78+        }
 79+        std::copy(first, last, data_);
 80+        size_ = new_size;
 81+    }
 82+
 83+    constexpr void clear() noexcept { size_ = 0; }
 84+
 85+    constexpr B& operator[](std::size_t i) { return data_[i]; }
 86+    constexpr B operator[](std::size_t i) const { return data_[i]; }
 87+
 88+    constexpr B* data() noexcept { return data_; }
 89+    constexpr const B* data() const noexcept { return data_; }
 90+
 91+    constexpr std::size_t size() const noexcept { return size_; }
 92+    constexpr bool empty() const noexcept { return size_ == 0; }
 93+    constexpr std::size_t capacity() const noexcept { return N; }
 94+
 95+    constexpr B* begin() noexcept { return data_; }
 96+    constexpr B* end() noexcept { return data_ + size_; }
 97+    constexpr const B* begin() const noexcept { return data_; }
 98+    constexpr const B* end() const noexcept { return data_ + size_; }
 99+
100+    constexpr bool operator==(const ByteArrayVec& other) const noexcept { return std::equal(begin(), end(), other.begin(), other.end()); }
101+    constexpr std::strong_ordering operator<=>(const ByteArrayVec& other) const noexcept
102+    {
103+        return std::lexicographical_compare_three_way(begin(), end(), other.begin(), other.end());
104+    }
105+};
106+
107+
108 /**
109  * Network address.
110  */
111@@ -116,7 +203,7 @@ protected:
112      * Raw representation of the network address.
113      * In network byte order (big endian) for IPv4 and IPv6.
114      */
115-    prevector<ADDR_IPV6_SIZE, uint8_t> m_addr{ADDR_IPV6_SIZE, 0x0};
116+    ByteArrayVec<ADDR_IPV6_SIZE, uint8_t> m_addr{};
117 
118     /**
119      * Network to which this address belongs.

refactor: remove unnecessary std::move for trivially copyable types #34514

Problem

Fix

Reviews

Conflicts