maflcko
commented at 8:41 PM on January 13, 2025:
member
Top level const in declarations is problematic for many reasons:
It is often a typo, where one wanted to denote a const reference. For example bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, ... is missing the &. This will create a redundant copy of the value.
In constructors it prevents move construction.
It can incorrectly imply some data is const, like in an imaginary example std::span<int> Shuffle(const std::span<int>);, where the ints are not const.
The compiler ignores the const from the declaration in the implementation.
It isn't used consistently anyway, not even on the same line.
Fix some issues by:
Using a const reference to avoid a copy, where read-only of the value is intended. This is only done for values that may be expensive to copy.
Using move-construction to avoid a copy
Applying readability-avoid-const-params-in-decls via clang-tidy
DrahtBot
commented at 8:41 PM on January 13, 2025:
contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
#33663 (net: Filter addrman during address selection via AddrPolicy to avoid underfill by waketraindev)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#33008 (wallet: support bip388 policy with external signer by Sjors)
#32958 (wallet/refactor: change PSBTError to PSBTResult and remove std::optionalcommon::PSBTResult and return common::PSBTResult by kevkevinpal)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading 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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
DrahtBot renamed this: refactor: Enforce readability-avoid-const-params-in-decls refactor: Enforce readability-avoid-const-params-in-decls on Jan 13, 2025
DrahtBot added the label Refactoring on Jan 13, 2025
hebasto
commented at 9:01 AM on January 14, 2025:
member
Concept ACK.
maflcko force-pushed on Jan 14, 2025
DrahtBot added the label Needs rebase on Jan 16, 2025
maflcko force-pushed on Jan 20, 2025
DrahtBot removed the label Needs rebase on Jan 20, 2025
fanquake
commented at 8:51 PM on February 20, 2025:
member
Concept ACK - has enforcement, few conflicts, added to 30.x.
fanquake added this to the milestone 30.0 on Feb 20, 2025
maflcko force-pushed on Feb 20, 2025
DrahtBot added the label Needs rebase on Mar 20, 2025
maflcko force-pushed on Mar 20, 2025
DrahtBot removed the label Needs rebase on Mar 20, 2025
in
src/external_signer.h:47
in
fafe09b016outdated
43 | @@ -44,7 +44,7 @@ class ExternalSigner
44 | //! @param[in,out] signers vector to which new signers (with a unique master key fingerprint) are added
45 | //! @param chain "main", "test", "regtest" or "signet"
46 | //! @returns success
47 | - static bool Enumerate(const std::string& command, std::vector<ExternalSigner>& signers, const std::string chain); 48 | + static bool Enumerate(const std::string& command, std::vector<ExternalSigner>& signers, const std::string& chain);
In commit "refactor: Avoid copies by using const references or by move-construction" (fafe09b0161c8c8d22a1e2e1bad0428b51fca813)
This change seems reasonable but would point out that sometimes short string copies like this can be intentional:
#24306 (review). I'd probably go with this change, but I don't actually know if copying might be better.
I'd be surprised if anything made a difference here in this function and I think anything is fine here (std::string copy, const std::string& reference, or even a span or std::string_view).
The goal of this pull is to avoid const params in declarations, not to avoid const params in definitions.
I can understand the confusion, so I am happy to do any of the following:
Close this pull
Just take the first commit and drop the second one
Add a commit to remove const in definitions as well, though, this at this point we may enter the territory where it isn't worth it to spend review time on this refactor
Was the const std::string_view kept in the .cpp file on purpose?
IMO top-level consts in method declarations are always harmful because they are confusing, misleading, unchecked by compilers, and noisy.
But top-level consts in method definitions can be good or harmful or neither or both depending on the context. They can be good if they prevent bugs from mutating parameters, or make it clearer that variables won't change. They can be bad if they prevent move optimizations, or make code more complicated by requiring declarations of similar extra variables.
This point of this commit fa8e57951abc5c737ab68694b8f6aa77551faa6b is to enable the readability-avoid-const-params-in-decls check, so it makes sense to me if it only updates declarations and leaves most definitions alone.
Yes, agree in general - this example seemed different, that's why I asked.
I hope we can reenable the clang-tidy in a next PR, I also think it was a useful change.
l0rinc
commented at 7:41 PM on March 28, 2025:
contributor
Concept ACK - this should clear up a few const usages.
After rebasing I found a few other similar ones, but not sure if they were ignored on purpose or not.
Please consider the ones that are related to this change - and just skip the ones that you've ignored on purpose.
<details>
<summary>Details</summary>
diff --git a/src/cuckoocache.h b/src/cuckoocache.h
index 8370179395..9dcceec739 100644
--- a/src/cuckoocache.h
+++ b/src/cuckoocache.h
@@ -471,7 +471,7 @@ public:
* flag is set
* [@returns](/bitcoin-bitcoin/contributor/returns/) true if the element is found, false otherwise
*/
- inline bool contains(const Element& e, const bool erase) const
+ inline bool contains(const Element& e, bool erase) const
{
std::array<uint32_t, 8> locs = compute_hashes(e);
for (const uint32_t loc : locs)
diff --git a/src/deploymentinfo.cpp b/src/deploymentinfo.cpp
index 185a7dcb54..237de65820 100644
--- a/src/deploymentinfo.cpp
+++ b/src/deploymentinfo.cpp
@@ -37,7 +37,7 @@ std::string DeploymentName(Consensus::BuriedDeployment dep)
return "";
}
-std::optional<Consensus::BuriedDeployment> GetBuriedDeployment(const std::string_view name)
+std::optional<Consensus::BuriedDeployment> GetBuriedDeployment(std::string_view name)
{
if (name == "segwit") {
return Consensus::BuriedDeployment::DEPLOYMENT_SEGWIT;
diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
index 2adeaea652..4d2fa5013b 100644
--- a/src/kernel/mempool_entry.h
+++ b/src/kernel/mempool_entry.h
@@ -207,7 +207,7 @@ struct TransactionInfo {
/* The block height the transaction entered the mempool */
const unsigned int txHeight;
- TransactionInfo(const CTransactionRef& tx, const CAmount& fee, const int64_t vsize, const unsigned int height)
+ TransactionInfo(const CTransactionRef& tx, const CAmount& fee, int64_t vsize, unsigned int height)
: m_tx{tx},
m_fee{fee},
m_virtual_transaction_size{vsize},
@@ -238,10 +238,10 @@ struct NewMempoolTransactionInfo {
const bool m_has_no_mempool_parents;
explicit NewMempoolTransactionInfo(const CTransactionRef& tx, const CAmount& fee,
- const int64_t vsize, const unsigned int height,
- const bool mempool_limit_bypassed, const bool submitted_in_package,
- const bool chainstate_is_current,
- const bool has_no_mempool_parents)
+ const int64_t vsize, unsigned int height,
+ bool mempool_limit_bypassed, bool submitted_in_package,
+ bool chainstate_is_current,
+ bool has_no_mempool_parents)
: info{tx, fee, vsize, height},
m_mempool_limit_bypassed{mempool_limit_bypassed},
m_submitted_in_package{submitted_in_package},
diff --git a/src/logging.h b/src/logging.h
index fdc12c79b3..de0e1cab7e 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -239,7 +239,7 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve
bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
template <typename... Args>
-inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
+inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags flag, BCLog::Level level, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
{
if (LogInstance().Enabled()) {
std::string log_msg;
diff --git a/src/netbase.h b/src/netbase.h
index d7c6e1c215..02b61a29de 100644
--- a/src/netbase.h
+++ b/src/netbase.h
@@ -60,7 +60,7 @@ class Proxy
public:
Proxy() : m_is_unix_socket(false), m_randomize_credentials(false) {}
explicit Proxy(const CService& _proxy, bool _randomize_credentials = false) : proxy(_proxy), m_is_unix_socket(false), m_randomize_credentials(_randomize_credentials) {}
- explicit Proxy(const std::string path, bool _randomize_credentials = false) : m_unix_socket_path(path), m_is_unix_socket(true), m_randomize_credentials(_randomize_credentials) {}
+ explicit Proxy(std::string path, bool _randomize_credentials = false) : m_unix_socket_path(std::move(path)), m_is_unix_socket(true), m_randomize_credentials(_randomize_credentials) {}
CService proxy;
std::string m_unix_socket_path;
diff --git a/src/node/miner.h b/src/node/miner.h
index c09e9eb5d1..2940df5435 100644
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -220,7 +220,7 @@ private:
* accounts for the BIP94 timewarp rule, so does not necessarily reflect the
* consensus limit.
*/
-int64_t GetMinimumTime(const CBlockIndex* pindexPrev, const int64_t difficulty_adjustment_interval);
+int64_t GetMinimumTime(const CBlockIndex* pindexPrev, int64_t difficulty_adjustment_interval);
int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
diff --git a/src/qt/addresstablemodel.h b/src/qt/addresstablemodel.h
index 877b01d882..595b815afd 100644
--- a/src/qt/addresstablemodel.h
+++ b/src/qt/addresstablemodel.h
@@ -90,7 +90,7 @@ public:
QString GetWalletDisplayName() const;
private:
- WalletModel* const walletModel;
+ WalletModel* walletModel;
AddressTablePriv *priv = nullptr;
QStringList columns;
EditStatus editStatus = OK;
diff --git a/src/qt/walletview.h b/src/qt/walletview.h
index 475085044d..687f22234a 100644
--- a/src/qt/walletview.h
+++ b/src/qt/walletview.h
@@ -56,7 +56,7 @@ private:
//! The wallet model represents a bitcoin wallet, and offers access to
//! the list of transactions, address book and sending functionality.
//!
- WalletModel* const walletModel;
+ WalletModel* walletModel;
OverviewPage *overviewPage;
QWidget *transactionsPage;
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 041186cae0..feb650bcf2 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -1773,7 +1773,7 @@ std::optional<std::pair<std::vector<unsigned char>, int>> ParseHexStrEnd(std::sp
/** BuildBack pops the last two elements off `constructed` and wraps them in the specified Fragment */
template<typename Key>
-void BuildBack(const MiniscriptContext script_ctx, Fragment nt, std::vector<NodeRef<Key>>& constructed, const bool reverse = false)
+void BuildBack(const MiniscriptContext script_ctx, Fragment nt, std::vector<NodeRef<Key>>& constructed, bool reverse = false)
{
NodeRef<Key> child = std::move(constructed.back());
constructed.pop_back();
@@ -1814,7 +1814,7 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1);
// Parses a multi() or multi_a() from its string representation. Returns false on parsing error.
- const auto parse_multi_exp = [&](std::span<const char>& in, const bool is_multi_a) -> bool {
+ const auto parse_multi_exp = [&](std::span<const char>& in, bool is_multi_a) -> bool {
const auto max_keys{is_multi_a ? MAX_PUBKEYS_PER_MULTI_A : MAX_PUBKEYS_PER_MULTISIG};
const auto required_ctx{is_multi_a ? MiniscriptContext::TAPSCRIPT : MiniscriptContext::P2WSH};
if (ctx.MsContext() != required_ctx) return false;
diff --git a/src/streams.h b/src/streams.h
index 20bdaf2c06..f6bc0478a3 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -547,7 +547,7 @@ public:
//! Move the read position ahead in the stream to the given position.
//! Use SetPos() to back up in the stream, not SkipTo().
- void SkipTo(const uint64_t file_pos)
+ void SkipTo(uint64_t file_pos)
{
assert(file_pos >= m_read_pos);
while (m_read_pos < file_pos) AdvanceStream(file_pos - m_read_pos);
diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
index 297595a9cf..92ffacdfdc 100644
--- a/src/test/argsman_tests.cpp
+++ b/src/test/argsman_tests.cpp
@@ -52,7 +52,7 @@ BOOST_AUTO_TEST_CASE(util_datadir)
struct TestArgsManager : public ArgsManager
{
TestArgsManager() { m_network_only_args.clear(); }
- void ReadConfigString(const std::string str_config)
+ void ReadConfigString(const std::string& str_config)
{
std::istringstream streamConfig(str_config);
{
@@ -63,7 +63,7 @@ struct TestArgsManager : public ArgsManager
std::string error;
BOOST_REQUIRE(ReadConfigStream(streamConfig, "", error));
}
- void SetNetworkOnlyArg(const std::string arg)
+ void SetNetworkOnlyArg(const std::string& arg)
{
LOCK(cs_args);
m_network_only_args.insert(arg);
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 52d7bd0efc..3bea74d9f7 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -258,7 +258,7 @@ struct TestChain100Setup : public TestingSetup {
* be used in "hot loops", for example fuzzing or benchmarking.
*/
template <class T = const BasicTestingSetup>
-std::unique_ptr<T> MakeNoLogFileContext(const ChainType chain_type = ChainType::REGTEST, TestOpts opts = {})
+std::unique_ptr<T> MakeNoLogFileContext(ChainType chain_type = ChainType::REGTEST, TestOpts opts = {})
{
opts.extra_args = Cat(
{
diff --git a/src/uint256.cpp b/src/uint256.cpp
index 2756a7f5cd..f9fe8187a6 100644
--- a/src/uint256.cpp
+++ b/src/uint256.cpp
@@ -18,7 +18,7 @@ std::string base_blob<BITS>::GetHex() const
}
template <unsigned int BITS>
-void base_blob<BITS>::SetHexDeprecated(const std::string_view str)
+void base_blob<BITS>::SetHexDeprecated(std::string_view str)
{
std::fill(m_data.begin(), m_data.end(), 0);
diff --git a/src/util/strencodings.h b/src/util/strencodings.h
index fd713a555c..1875810ab8 100644
--- a/src/util/strencodings.h
+++ b/src/util/strencodings.h
@@ -369,7 +369,7 @@ std::optional<uint64_t> ParseByteUnits(std::string_view str, ByteUnit default_mu
namespace util {
/** consteval version of HexDigit() without the lookup table. */
-consteval uint8_t ConstevalHexDigit(const char c)
+consteval uint8_t ConstevalHexDigit(char c)
{
if (c >= '0' && c <= '9') return c - '0';
if (c >= 'a' && c <= 'f') return c - 'a' + 0xa;
diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h
index e029f39430..c5e4526412 100644
--- a/src/wallet/coinselection.h
+++ b/src/wallet/coinselection.h
@@ -93,7 +93,7 @@ public:
}
}
- COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, const CAmount fees)
+ COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, CAmount fees)
: COutput(outpoint, txout, depth, input_bytes, spendable, solvable, safe, time, from_me)
{
// if input_bytes is unknown, then fees should be 0, if input_bytes is known, then the fees should be a positive integer or 0 (input_bytes known and fees = 0 only happens in the tests)
@@ -355,7 +355,7 @@ private:
}
public:
- explicit SelectionResult(const CAmount target, SelectionAlgorithm algo)
+ explicit SelectionResult(CAmount target, SelectionAlgorithm algo)
: m_target(target), m_algo(algo) {}
SelectionResult() = delete;
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index 91dfd35479..322770e197 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -179,14 +179,14 @@ protected:
public:
explicit ScriptPubKeyMan(WalletStorage& storage) : m_storage(storage) {}
virtual ~ScriptPubKeyMan() = default;
- virtual util::Result<CTxDestination> GetNewDestination(const OutputType type) { return util::Error{Untranslated("Not supported")}; }
+ virtual util::Result<CTxDestination> GetNewDestination(OutputType type) { return util::Error{Untranslated("Not supported")}; }
virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; }
//! Check that the given decryption key is valid for this ScriptPubKeyMan, i.e. it decrypts all of the keys handled by it.
virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key) { return false; }
virtual bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) { return false; }
- virtual util::Result<CTxDestination> GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { return util::Error{Untranslated("Not supported")}; }
+ virtual util::Result<CTxDestination> GetReservedDestination(OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { return util::Error{Untranslated("Not supported")}; }
virtual void KeepDestination(int64_t index, const OutputType& type) {}
virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {}
diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h
index 3acaba0202..e584cd10e5 100644
--- a/src/wallet/test/util.h
+++ b/src/wallet/test/util.h
@@ -128,7 +128,7 @@ public:
std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase(MockableData records = {});
MockableDatabase& GetMockableDatabase(CWallet& wallet);
-ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success);
+ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, bool success);
} // namespace wallet
#endif // BITCOIN_WALLET_TEST_UTIL_H
diff --git a/src/zmq/zmqabstractnotifier.h b/src/zmq/zmqabstractnotifier.h
index 17fa7bbaa9..1cf54b8a78 100644
--- a/src/zmq/zmqabstractnotifier.h
+++ b/src/zmq/zmqabstractnotifier.h
@@ -35,7 +35,7 @@ public:
std::string GetAddress() const { return address; }
void SetAddress(const std::string &a) { address = a; }
int GetOutboundMessageHighWaterMark() const { return outbound_message_high_water_mark; }
- void SetOutboundMessageHighWaterMark(const int sndhwm) {
+ void SetOutboundMessageHighWaterMark(int sndhwm) {
if (sndhwm >= 0) {
outbound_message_high_water_mark = sndhwm;
}
</details>
maflcko removed this from the milestone 30.0 on Mar 31, 2025
maflcko renamed this: refactor: Enforce readability-avoid-const-params-in-decls refactor: Avoid copies by using const references or by move-construction on Mar 31, 2025
DrahtBot renamed this: refactor: Avoid copies by using const references or by move-construction refactor: Avoid copies by using const references or by move-construction on Mar 31, 2025
maflcko force-pushed on Mar 31, 2025
maflcko
commented at 8:18 AM on March 31, 2025:
member
Dropped the second commit, changed pull title and description, and rebased. Should be trivial to re-review.
l0rinc
commented at 9:18 AM on March 31, 2025:
contributor
crACKfa9cd7bd4f8b4bad4b01bb3709f91d2a651fe47d
<details>
<summary>Diff compared to previous review (excluding the dropped the last commit)</summary>
DrahtBot requested review from ryanofsky on Mar 31, 2025
ryanofsky approved
ryanofsky
commented at 9:24 PM on March 31, 2025:
contributor
Code review ACKfa9cd7bd4f8b4bad4b01bb3709f91d2a651fe47d. Since last review just extended first commit a little and dropped second commit fa8e57951abc5c737ab68694b8f6aa77551faa6b
I was actually more interested in the second commit fa8e57951abc5c737ab68694b8f6aa77551faa6b which enabled the readability-avoid-const-params-in-decls check than the first commit because second commit enabled a tidy check that could catch bugs and removed const keywords that were literally meaningless and misleading. The first commit also seems like a good change, though.
maflcko marked this as a draft on Apr 1, 2025
DrahtBot added the label Needs rebase on Apr 10, 2025
maflcko marked this as ready for review on Apr 22, 2025
maflcko force-pushed on Apr 22, 2025
DrahtBot removed the label Needs rebase on Apr 22, 2025
maflcko requested review from l0rinc on Apr 23, 2025
l0rinc
commented at 9:59 AM on April 23, 2025:
contributor
ACKfa19d8bee350fef088f5fabca38f09610fa8d20d
The rebase had to fix a Proxy merge conflict but otherwise is a clean cherry-pick of the previous readability-avoid-const-params-in-decls commit.
Q: Any reason for not including these const removals in the last commit?
blockheaderToJSON
blockToJSON
DeriveTarget
GetTarget
interfaces::BlockTemplate::waitNext
node::GetMinimumTime
wallet::CreateDescriptor
wallet::CWallet::ListAddrBookFunc
Please update the PR description now that readability-avoid-const-params-in-decls was reapplied.
DrahtBot requested review from ryanofsky on Apr 23, 2025
fanquake
commented at 1:14 PM on April 23, 2025:
member
[09:08:35.465] /ci_container_base/src/rpc/util.h:527:50: error: parameter 'pow_limit' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls,-warnings-as-errors]
[09:08:35.465] 527 | uint256 GetTarget(const CBlockIndex& blockindex, const uint256 pow_limit);
DrahtBot added the label CI failed on Apr 23, 2025
DrahtBot
commented at 1:29 PM on April 23, 2025:
contributor
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
DrahtBot added the label Needs rebase on May 7, 2025
maflcko force-pushed on Jul 10, 2025
maflcko
commented at 1:32 PM on July 10, 2025:
member
Please update the PR description now that readability-avoid-const-params-in-decls was reapplied.
DrahtBot added the label Needs rebase on Jul 25, 2025
rkrux
commented at 7:36 AM on October 21, 2025:
contributor
ACKfa9b7f0630691cf6b0add88f646bbcae6466eeaa
Specially for the first two reasons mentioned in the PR description.
ryanofsky approved
ryanofsky
commented at 7:27 PM on December 17, 2025:
contributor
Code review ACKfa9b7f0630691cf6b0add88f646bbcae6466eeaa. Looks like this needs to be rebased again but latest version does look good, and this would be a nice change
pablomartin4btc
commented at 5:10 PM on December 24, 2025:
member
cr-ACKfa9b7f0630691cf6b0add88f646bbcae6466eeaa
The two-commit split (semantic fixes vs style/clang-tidy) makes sense.
I initially wondered whether further splitting (e.g. copy-avoidance vs const-cleanup) would help review, but given how intertwined these changes are, the current structure seems reasonable.
For reference, here is a quick summary of the const-related changes that helped me to understand and follow them:
Original
After Change
Reason
const T x (expensive type)
const T& x
Avoid expensive copy (often a missing &)
const T x (cheap type)
T x
Remove meaningless top-level const
const& preventing move
T x or T&&
Enable move construction
Top-level const in declarations
Removed
Misleading, ignored by compiler, often a typo
Top-level const in definitions
May remain
Implementation detail / self-documentation
const T implying deep const
Removed
Prevents false immutability assumptions
Inconsistent const usage
Normalized
Improves readability and style consistency
Also, noticed some comments from the PR's author that perhas worth mentioning in the description:
“The goal of this pull is to avoid const params in declarations, not to avoid const params in definitions.”,
"Performance improvements (by the first commit). However, there are no benchmarks..." (avoid unnecessary copies and enable move construction; these changes remove redundant work in frequently executed paths.),
"Readability and style related improvements. However, they are not enforced consistently..." (improves const-correctness and readability where touched, but does not attempt a full project-wide normalization.).
maflcko force-pushed on Jan 8, 2026
maflcko force-pushed on Jan 8, 2026
DrahtBot added the label CI failed on Jan 8, 2026
DrahtBot
commented at 8:50 PM on January 8, 2026:
contributor
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
maflcko
commented at 9:05 PM on January 8, 2026:
member
Rebased. Looks like there have only been a few conflicts and it should be not too hard to re-review with a range-diff.
DrahtBot removed the label Needs rebase on Jan 8, 2026
DrahtBot removed the label CI failed on Jan 8, 2026
I'd be surprised if anything made a difference here in this function and I think anything is fine here (std::string copy, const std::string& reference, or even a span or std::string_view).
So I'll leave as-is for now, to leave the code minimal and untouched in that line.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-05-02 06:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
