refactor: remove unnecessary std::move for trivially copyable types by l0rinc · Pull Request #34514 · bitcoin/bitcoin

l0rinc · 2026-02-05T13:27:48Z

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 doc: Clarify why performance-move-const-arg.CheckTriviallyCopyableMove=false #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 · 2026-02-05T13:28:40Z

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:

#34778 (logging: rewrite macros to add ratelimit option, avoid unused strprintf, clarify confusing errors by ryanofsky)
#34038 (logging: replace -loglevel with -trace, various API improvements by ajtowns)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#30342 (kernel, logging: Pass Logger instances to kernel objects 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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

LogInstance().LogPrintStr(msg, loc, category, level, false) in src/test/logging_tests.cpp

^{2026-02-19 23:12:09}

maflcko

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?

maflcko · 2026-02-05T14:26:12Z

src/txmempool.cpp

-CTxMemPool::CTxMemPool(Options opts, bilingual_str& error)
-    : m_opts{Flatten(std::move(opts), error)}
+CTxMemPool::CTxMemPool(const Options& opts, bilingual_str& error)
+    : m_opts{Flatten(opts, error)}


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)

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?

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

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.

hodlinator · 2026-02-05T14:54:22Z

src/logging.h

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

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

hodlinator · 2026-02-05T15:02:55Z

src/validation.cpp

    return nSubsidy;
 }

 CoinsViews::CoinsViews(DBParams db_params, CoinsViewOptions options)


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

Suggested change

CoinsViews::CoinsViews(DBParams db_params, CoinsViewOptions options)

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

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.

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:

bitcoin/src/txdb.h

Lines 26 to 31 in 32f00a5

struct CoinsViewOptions {

//! Maximum database write batch size in bytes.

size_t batch_write_bytes{DEFAULT_DB_CACHE_BATCH};

//! If non-zero, randomly exit when the database is flushed with (1/ratio) probability.

int simulate_crash_ratio{0};

};

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.

Reverted all Option moves

purpleKarrot · 2026-02-05T15:50:30Z

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 · 2026-02-05T15:54:35Z

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 faad673 (zirka 2022)

hodlinator · 2026-02-05T19:32:13Z

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 · 2026-02-06T07:50:45Z

Fixed the docs in #34523

l0rinc · 2026-02-06T10:25:50Z

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.

maflcko · 2026-02-06T13:56:36Z

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.

bvbfan · 2026-02-08T08:08:20Z

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

…llyCopyableMove=false fa88ac3 doc: Clarify why performance-move-const-arg.CheckTriviallyCopyableMove=false (MarcoFalke) Pull request description: Without this doc, there is a risk that the setting will be turned off, see #34514. The reason to disable it is to catch logic bugs, even on trivially copyable types: ```cpp #include <utility> void Eat(int&& food) { food = 0; }; int main() { int food{2}; Eat(std::move(food)); Eat(std::move(food)); // This should err } ``` ACKs for top commit: l0rinc: ACK fa88ac3 hebasto: ACK fa88ac3. sedited: ACK fa88ac3 Tree-SHA512: d1bda846a10190a2936084a06bd87418c6a3e4ababc298e4beb9bc9e1190bff430cbe973475d634eda5ef7863571c89bfa4b78ff63fcbd9ac10c42fd9d5fa23a

l0rinc · 2026-02-09T14:03:40Z

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 · 2026-02-09T18:23:37Z

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 · 2026-02-10T09:13:32Z

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 · 2026-02-10T10:18:56Z

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 · 2026-02-10T16:59:26Z

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:

a diff

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.

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

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

… in constructors

fanquake changed the title ~~L0rinc/move const arg trivcopy~~ refactor: enable move-const-arg for trivially-copyable types Feb 5, 2026

DrahtBot added the Refactoring label Feb 5, 2026

maflcko reviewed Feb 5, 2026

View reviewed changes

hodlinator reviewed Feb 5, 2026

View reviewed changes

maflcko mentioned this pull request Feb 6, 2026

doc: Clarify why performance-move-const-arg.CheckTriviallyCopyableMove=false #34523

Merged

DrahtBot mentioned this pull request Feb 6, 2026

refactor: separate log generation from log handling #34465

Merged

l0rinc force-pushed the l0rinc/move-const-arg-trivcopy branch from 32f00a5 to 6c7b943 Compare February 6, 2026 10:22

l0rinc changed the title ~~refactor: enable move-const-arg for trivially-copyable types~~ refactor: remove unnecessary std::move for trivially copyable types Feb 6, 2026

l0rinc requested review from andrewtoth, hodlinator, maflcko and purpleKarrot February 6, 2026 13:53

DrahtBot added the Needs rebase label Feb 7, 2026

l0rinc force-pushed the l0rinc/move-const-arg-trivcopy branch from 6c7b943 to c7782b7 Compare February 8, 2026 06:50

DrahtBot removed the Needs rebase label Feb 8, 2026

DrahtBot mentioned this pull request Feb 9, 2026

coins: use dirty entry count for flush warnings and disk space checks #33512

Merged

l0rinc force-pushed the l0rinc/move-const-arg-trivcopy branch from c7782b7 to 1414bfe Compare February 9, 2026 14:02

DrahtBot mentioned this pull request Feb 14, 2026

wallet, logging: Replace WalletLogPrintf() with LogInfo() #30343

Draft

DrahtBot added the Needs rebase label Feb 19, 2026

l0rinc force-pushed the l0rinc/move-const-arg-trivcopy branch from 1414bfe to 5152fdb Compare February 19, 2026 23:11

l0rinc and others added 4 commits February 20, 2026 00:11

coins: avoid moving COutPoint in snapshot load

f44a012

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

rpc: remove std::move on RPCArg::Type and RPCResult::Type enums…

528b24c

… in constructors

txgraph: remove unnecessary std::move on primitive members

597fd6e

l0rinc force-pushed the l0rinc/move-const-arg-trivcopy branch from 5152fdb to 597fd6e Compare February 19, 2026 23:11

DrahtBot added CI failed and removed CI failed Needs rebase labels Feb 19, 2026

DrahtBot mentioned this pull request Mar 10, 2026

logging: rewrite macros to add ratelimit option, avoid unused strprintf, clarify confusing errors #34778

Open

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

	struct CoinsViewOptions {
	//! Maximum database write batch size in bytes.
	size_t batch_write_bytes{DEFAULT_DB_CACHE_BATCH};
	//! If non-zero, randomly exit when the database is flushed with (1/ratio) probability.
	int simulate_crash_ratio{0};
	};

Conversation

l0rinc commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Uh oh!

DrahtBot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purpleKarrot Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purpleKarrot commented Feb 5, 2026

Uh oh!

maflcko commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hodlinator commented Feb 5, 2026

Uh oh!

maflcko commented Feb 6, 2026

Uh oh!

l0rinc commented Feb 6, 2026

Uh oh!

maflcko commented Feb 6, 2026

Uh oh!

bvbfan commented Feb 8, 2026

Uh oh!

l0rinc commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvbfan commented Feb 9, 2026

Uh oh!

maflcko commented Feb 10, 2026

Uh oh!

l0rinc commented Feb 10, 2026

Uh oh!

maflcko commented Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

l0rinc commented Feb 5, 2026 •

edited

Loading

DrahtBot commented Feb 5, 2026 •

edited

Loading

purpleKarrot Feb 5, 2026 •

edited

Loading

l0rinc Feb 6, 2026 •

edited

Loading

maflcko commented Feb 5, 2026 •

edited

Loading

l0rinc commented Feb 9, 2026 •

edited

Loading