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

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.

Type	Reviewers
ACK	hodlinator

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34865 (logging: better use of log::Entry internally by stickies-v)
#34806 (refactor: logging: Various API improvements by ajtowns)
#34778 (logging: rewrite macros to add ratelimit option, avoid unused strprintf, clarify confusing errors by ryanofsky)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#29256 (log, refactor: Allow log macros to accept context arguments by ryanofsky)

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

hodlinator commented at 7:27 PM on March 19, 2026:

Cheers, but I think the static_assert is stating something about the the implementation and so should be located in it, instead of in a test. Maybe put it in the body of BCLog::Logger::LogPrintStr_(), or possibly in logging.h/log.h.

l0rinc commented at 2:54 AM on March 20, 2026:

I don't like spreading around tests, I can make it a regular assert if that helps

hodlinator commented at 2:02 PM on March 24, 2026:

I don't see this static assertion as an external test. It is verifying a constraint of the implementation and helping to document why we prefer const ref over r-value ref right next to it rather than far away in a test file.

See static_assert usage in /src/util/bitset.h for example.

l0rinc commented at 11:02 AM on March 25, 2026:

possibly in logging.h/log.h

Ok, added it after the declaration of SourceLocation

hodlinator commented at 11:57 AM on March 25, 2026:

Thanks!

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:

CoinsViews::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

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

template<typename T>
std::conditional_t<std::is_trivially_copyable_v<T>, T, T&&>
move_if_not_trivialy_copyable(T& x) {
    if constexpr (std::is_trivially_copyable_v<T>) {
        return x;
    }
    return std::move(x);
}

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):

src/httpserver.cpp:131:21: warning: pass by value and use std::move [modernize-pass-by-value]
src/httpserver.cpp:131:60: warning: pass by value and use std::move [modernize-pass-by-value]
src/i2p.cpp:122:18: warning: pass by value and use std::move [modernize-pass-by-value]
src/i2p.cpp:130:45: warning: pass by value and use std::move [modernize-pass-by-value]
src/net.cpp:3374:20: warning: pass by value and use std::move [modernize-pass-by-value]
src/net.cpp:3968:14: warning: pass by value and use std::move [modernize-pass-by-value]
src/test/bip32_tests.cpp:29:25: warning: pass by value and use std::move [modernize-pass-by-value]
src/test/blockfilter_index_tests.cpp:317:72: warning: pass by value and use std::move [modernize-pass-by-value]
src/test/util/net.cpp:341:18: warning: pass by value and use std::move [modernize-pass-by-value]
src/test/util/net.cpp:341:48: warning: pass by value and use std::move [modernize-pass-by-value]
src/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:

diff --git a/src/netaddress.cpp b/src/netaddress.cpp
index fb2c254076..201c228283 100644
--- a/src/netaddress.cpp
+++ b/src/netaddress.cpp
@@ -26,6 +26,8 @@ using util::HasPrefix;
 
 CNetAddr::BIP155Network CNetAddr::GetBIP155Network() const
 {
+    static_assert(std::is_trivially_copyable_v<CNetAddr>);
+    static_assert(std::is_trivially_copyable_v<decltype(CNetAddr::m_addr)>);
     switch (m_net) {
     case NET_IPV4:
         return BIP155Network::IPV4;
diff --git a/src/netaddress.h b/src/netaddress.h
index 2191da54b7..86463879b1 100644
--- a/src/netaddress.h
+++ b/src/netaddress.h
@@ -106,6 +106,93 @@ static constexpr uint16_t I2P_SAM31_PORT{0};
 
 std::string OnionToString(std::span<const uint8_t> addr);
 
+#include <algorithm>
+#include <cstddef>
+#include <stdexcept>
+
+template <std::size_t N, BasicByte B>
+class ByteArrayVec
+{
+private:
+    B data_[N]{};
+    std::size_t size_{0};
+
+public:
+    constexpr ByteArrayVec() = default;
+
+    constexpr void push_back(B b)
+    {
+        if (size_ >= N) [[unlikely]] {
+            throw std::out_of_range("ByteArrayVec capacity exceeded");
+        }
+        data_[size_++] = b;
+    }
+
+    constexpr void pop_back()
+    {
+        if (size_ == 0) [[unlikely]] {
+            throw std::out_of_range("ByteArrayVec underflow: pop_back called on empty container");
+        }
+        --size_;
+    }
+
+    constexpr void resize(std::size_t new_size)
+    {
+        if (new_size > N) [[unlikely]] {
+            throw std::out_of_range("ByteArrayVec resize exceeds capacity");
+        }
+        if (new_size > size_) {
+            // Zero-initialize the appended bytes
+            std::fill_n(data_ + size_, new_size - size_, B{0});
+        }
+        size_ = new_size;
+    }
+
+    constexpr void assign(std::size_t count, B value)
+    {
+        if (count > N) [[unlikely]] {
+            throw std::out_of_range("ByteArrayVec assign exceeds capacity");
+        }
+        std::fill_n(data_, count, value);
+        size_ = count;
+    }
+
+    template <std::input_iterator It>
+    constexpr void assign(It first, It last)
+    {
+        const auto new_size = static_cast<std::size_t>(std::distance(first, last));
+        if (new_size > N) [[unlikely]] {
+            throw std::out_of_range("ByteArrayVec assign range exceeds capacity");
+        }
+        std::copy(first, last, data_);
+        size_ = new_size;
+    }
+
+    constexpr void clear() noexcept { size_ = 0; }
+
+    constexpr B& operator[](std::size_t i) { return data_[i]; }
+    constexpr B operator[](std::size_t i) const { return data_[i]; }
+
+    constexpr B* data() noexcept { return data_; }
+    constexpr const B* data() const noexcept { return data_; }
+
+    constexpr std::size_t size() const noexcept { return size_; }
+    constexpr bool empty() const noexcept { return size_ == 0; }
+    constexpr std::size_t capacity() const noexcept { return N; }
+
+    constexpr B* begin() noexcept { return data_; }
+    constexpr B* end() noexcept { return data_ + size_; }
+    constexpr const B* begin() const noexcept { return data_; }
+    constexpr const B* end() const noexcept { return data_ + size_; }
+
+    constexpr bool operator==(const ByteArrayVec& other) const noexcept { return std::equal(begin(), end(), other.begin(), other.end()); }
+    constexpr std::strong_ordering operator<=>(const ByteArrayVec& other) const noexcept
+    {
+        return std::lexicographical_compare_three_way(begin(), end(), other.begin(), other.end());
+    }
+};
+
+
 /**
  * Network address.
  */
@@ -116,7 +203,7 @@ protected:
      * Raw representation of the network address.
      * In network byte order (big endian) for IPv4 and IPv6.
      */
-    prevector<ADDR_IPV6_SIZE, uint8_t> m_addr{ADDR_IPV6_SIZE, 0x0};
+    ByteArrayVec<ADDR_IPV6_SIZE, uint8_t> m_addr{};
 
     /**
      * Network to which this address belongs.

</details>

DrahtBot added the label Needs rebase on Feb 19, 2026

l0rinc force-pushed on Feb 19, 2026

DrahtBot added the label CI failed on Feb 19, 2026

DrahtBot removed the label CI failed on Feb 20, 2026

DrahtBot removed the label Needs rebase on Feb 20, 2026

DrahtBot added the label Needs rebase on Mar 18, 2026

l0rinc force-pushed on Mar 18, 2026

DrahtBot removed the label Needs rebase on Mar 18, 2026

hodlinator commented at 7:36 PM on March 19, 2026: contributor

Reviewed 1b7fc8c3b49a8ba7fe87fa8042aff87006ef31cf

Curious to see feedback on my inline comment. Looks fine otherwise.

DrahtBot added the label Needs rebase on Mar 24, 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>

2e341e1041

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_`.

412c8dd15a

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

txgraph: remove unnecessary `std::move` on primitive members 10f4d933e6

l0rinc force-pushed on Mar 25, 2026

DrahtBot removed the label Needs rebase on Mar 25, 2026

hodlinator approved

hodlinator commented at 12:02 PM on March 25, 2026: contributor

ACK 10f4d933e605709b4fd4c92e9ec2860c60acd823

std::move() wasn't really doing anything for these types, and we don't increase cognitive complexity by adding that much alive scope after the no longer moved variables.