A follow-up of #31305.
The clang-tidy check can be run via:
0cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc)
1
2run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,performance-unnecessary-copy-initialization' | grep -v 'clang-tidy'
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31364.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33477196860
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.
https://cirrus-ci.com/task/4898976128827392:
0 /ci_container_base/src/qt/bitcoinunits.cpp:169:13: error: the variable 'whole' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [performance-unnecessary-copy-initialization,-warnings-as-errors]
1 169 | QString whole = parts[0];
2 | ^
3 | const &
the variable ‘whole’ is copy-constructed
Thanks, had to enable GUI as well (and clean the previous build directory, otherwise it’s not detected at all for some reason):
0git clean -fxd \
1&& cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_GUI=ON \
2&& cmake --build build -j$(nproc) \
3&& run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,performance-unnecessary-copy-initialization' | grep -v 'clang-tidy'
772@@ -773,8 +773,10 @@ static void CheckAddCoinBase(CAmount base_value, CAmount cache_value, CAmount mo
773 template <typename... Args>
774 static void CheckAddCoin(Args&&... args)
775 {
776- for (const CAmount base_value : {ABSENT, SPENT, VALUE1})
777+ for (const CAmount base_value : {ABSENT, SPENT, VALUE1}) {
This is masking a legitimate bug, it just happens to be ok for now because of the types involved.
It shouldn’t be marked nolint, rather it should be fixed: drop the forward and pass by const Args&.
review ACK 3305972f7bfd78181566e4297891c2dd7cae0f2 🏀
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 3305972f7bfd78181566e4297891c2dd7cae0f2 🏀
3H19cz7/LVBZwyyKNtknICAMAqJ4ZGwaiVn8WISNuKzneVEWedwz79LgzTplT581IvdKhojcgbIsFQ53gtK2WCg==
61@@ -62,7 +62,7 @@ static void SignTransactionSingleInput(benchmark::Bench& bench, InputType input_
62 bench.minEpochIterations(100).run([&] {
63 CMutableTransaction tx{unsigned_tx};
64 std::map<COutPoint, Coin> coins;
65- CScript prev_spk = prev_spks[(iter++) % prev_spks.size()];
66+ const CScript& prev_spk = prev_spks[(iter++) % prev_spks.size()];
The prev_spk variable is passed to the CTxOut ctor, which takes a copy rather than a const reference.
This raises a concern about the correctness of the change, doesn’t it?
The
prev_spkvariable is passed to theCTxOutctor, which takes a copy rather than a const reference.This raises a concern about the correctness of the change, doesn’t it?
It is possible to create a copy from a const reference. Previously a copy was taken from a mutable reference.
The change here only affects the code in this scope, to avoid a second copy in this line.
Not sure what your question is, can you rephrase it?
@hebasto are you asking because CScript is trivially copyable, but https://clang.llvm.org/extra/clang-tidy/checks/performance/unnecessary-copy-initialization.html claims
Finds local variable declarations that are initialized using the copy constructor of a non-trivially-copyable type but it would suffice to obtain a const reference.
?
The
prev_spkvariable is passed to theCTxOutctor…
The ctor treats CScript scriptPubKeyIn parameter as const. Everything is OK.
Sorry for the noise.
SetNull can clear it internally, but it’s copied in the constructor, that’s why it’s safe, isn’t it?
