refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors #31364

pull l0rinc wants to merge 1 commits into bitcoin:master from l0rinc:l0rinc/performance-unnecessary-copy-initialization changing 4 files +8 −8
  1. l0rinc commented at 6:39 pm on November 24, 2024: contributor

    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'
    
  2. DrahtBot commented at 6:39 pm on November 24, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31364.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, hebasto, achow101
    Ignored review theuni

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Refactoring on Nov 24, 2024
  4. maflcko commented at 11:51 am on November 25, 2024: member
    I don’t think you need to repeat the full diff (or a variation of it) in the commit message. If someone was interested in the detailed diff, they could just look at the diff.
  5. hebasto commented at 11:58 am on November 25, 2024: member

    Concept ACK.

    I’d prefer if this PR was based on #31306 to ensure that CI checks that all cases are addressed.

  6. l0rinc force-pushed on Nov 25, 2024
  7. l0rinc force-pushed on Nov 25, 2024
  8. l0rinc commented at 12:52 pm on November 25, 2024: contributor
    Rebased on top of #31306 and updated commit message
  9. DrahtBot added the label CI failed on Nov 25, 2024
  10. DrahtBot commented at 1:52 pm on November 25, 2024: contributor

    🚧 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.

  11. hebasto commented at 1:55 pm on November 25, 2024: member

    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  &
    
  12. l0rinc force-pushed on Nov 25, 2024
  13. l0rinc commented at 2:27 pm on November 25, 2024: contributor

    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'
    
  14. DrahtBot removed the label CI failed on Nov 25, 2024
  15. in src/test/coins_tests.cpp:776 in 3c259019ff outdated
    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}) {
    


    theuni commented at 6:29 pm on November 25, 2024:

    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&.


    l0rinc commented at 6:35 pm on November 25, 2024:
    Thanks for the review, please see #31306 (review) for those commits - this PR is based on it to make sure the CI checks the mentioned errors, your nack is likely directed at non-performance checks

    l0rinc commented at 7:12 pm on November 25, 2024:
    I’ve removed the commits from #31364
  16. theuni commented at 6:30 pm on November 25, 2024: member

    Concept NACK to these new tidy checks if sufficient care is not going to be taken.

    They should either be obviously/trivially safe or left alone.

    The offending commit (from #31306) has been removed.

  17. refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors 3305972f7b
  18. l0rinc force-pushed on Nov 25, 2024
  19. theuni approved
  20. theuni commented at 8:14 pm on November 25, 2024: member
    ACK the much more constrained 3305972f7bfd78181566e4297891c2dd7cae0f2b.
  21. maflcko commented at 9:56 am on November 26, 2024: member

    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==
    
  22. DrahtBot requested review from hebasto on Nov 26, 2024
  23. in src/bench/sign_transaction.cpp:65 in 3305972f7b
    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()];
    


    hebasto commented at 10:36 am on November 26, 2024:

    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?


    maflcko commented at 10:44 am on November 26, 2024:

    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?

    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?


    l0rinc commented at 10:50 am on November 26, 2024:

    @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.

    ?


    hebasto commented at 10:59 am on November 26, 2024:

    The prev_spk variable is passed to the CTxOut ctor…

    The ctor treats CScript scriptPubKeyIn parameter as const. Everything is OK.

    Sorry for the noise.


    l0rinc commented at 11:05 am on November 26, 2024:
    It does? SetNull can clear it internally, but it’s copied in the constructor, that’s why it’s safe, isn’t it?

    hebasto commented at 12:11 pm on November 26, 2024:
    I think so.
  24. DrahtBot requested review from hebasto on Nov 26, 2024
  25. hebasto approved
  26. hebasto commented at 11:00 am on November 26, 2024: member
    ACK 3305972f7bfd78181566e4297891c2dd7cae0f2b, tested with clang 19.1.5 + clang-tidy.
  27. achow101 commented at 8:42 pm on November 26, 2024: member
    ACK 3305972f7bfd78181566e4297891c2dd7cae0f2b
  28. achow101 merged this on Nov 26, 2024
  29. achow101 closed this on Nov 26, 2024


github-metadata-mirror

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: 2024-12-03 21:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me