clang-tidy: Add more `performance-*` checks and related fixes #26642

hebasto commented at 6:40 PM on December 5, 2022: member

No description provided.

DrahtBot commented at 6:40 PM on December 5, 2022: 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	martinus, TheCharlatan
Stale ACK	aureleoules

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26485 (RPC: Accept options as named-only parameters 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.

aureleoules commented at 7:39 PM on December 5, 2022: member

Concept ACK

in src/.clang-tidy:12 in 66b5bdd791 outdated

  20 | -performance-for-range-copy,
  21 | -performance-move-const-arg,
  22 | -performance-unnecessary-copy-initialization,
  23 | +performance-*,
  24 | +-performance-no-int-to-ptr,
  25 | +-performance-unnecessary-value-param,

aureleoules commented at 10:32 PM on December 5, 2022:

Is there a reason why these two are not enabled?

hebasto commented at 10:36 PM on December 5, 2022:

Is there a reason why they these two are not enabled?

The required fixes seem too invasive for this PR.

aureleoules approved

aureleoules commented at 10:33 PM on December 5, 2022: member

ACK 66b5bdd791aca024996db182cfdec4439895bec0

DrahtBot cross-referenced this on Dec 6, 2022 from issue ci: Integrate `bitcoin-tidy` clang-tidy plugin by fanquake

DrahtBot cross-referenced this on Dec 6, 2022 from issue refactor: Run type check against RPCArgs (1/2) by maflcko

DrahtBot cross-referenced this on Dec 6, 2022 from issue fuzz: Modify tx_pool_standard target to test package processing by chinggg

DrahtBot cross-referenced this on Dec 6, 2022 from issue descriptors: Be able to specify change and receiving in a single descriptor string by achow101

in src/.clang-tidy:10 in 6147104769 outdated

   6 | @@ -7,6 +7,7 @@ modernize-use-default-member-init,
   7 |  modernize-use-nullptr,
   8 |  performance-faster-string-find,
   9 |  performance-for-range-copy,
  10 | +performance-inefficient-string-concatenation,

maflcko commented at 3:24 PM on December 7, 2022:

Not sure. This makes the code pretty verbose and it is unclear whether append or strprintf should be used. Also, while this check is disabled for non-loops. In our code it is only triggered on errors in loops.

hebasto commented at 5:10 PM on December 7, 2022:

Thanks! Updated.

hebasto force-pushed on Dec 7, 2022

hebasto commented at 5:09 PM on December 7, 2022: member

Updated 66b5bdd791aca024996db182cfdec4439895bec0 -> 0524c30fbb05686a8cf8949a4b05d4a3c9a4c118 (pr26642.01 -> pr26642.02):

addressed @MarcoFalke comment

hebasto force-pushed on Dec 7, 2022

DrahtBot cross-referenced this on Dec 13, 2022 from issue doc: Fix incorrect sendmany RPC doc by maflcko

DrahtBot cross-referenced this on Dec 15, 2022 from issue [Draft / POC] Silent Payments by w0xlt

in src/.clang-tidy:12 in 61d7f1e451 outdated

   8 | @@ -9,6 +9,7 @@ performance-faster-string-find,
   9 |  performance-for-range-copy,
  10 |  performance-inefficient-vector-operation,
  11 |  performance-move-const-arg,
  12 | +performance-no-automatic-move,

maflcko commented at 5:53 PM on December 21, 2022:

Maybe split this out to make review/testing easier. It may be related to #25429 and https://github.com/bitcoin/bitcoin/pull/26308

hebasto commented at 3:48 PM on December 27, 2022:

Maybe split this out to make review/testing easier. It may be related to #25429 and #26308

Done in #26758.

DrahtBot cross-referenced this on Dec 22, 2022 from issue refactor: Avoid UniValue copies by maflcko

hebasto cross-referenced this on Dec 27, 2022 from issue refactor: Add `performance-no-automatic-move` clang-tidy check by hebasto

DrahtBot cross-referenced this on Jan 4, 2023 from issue test: add end-to-end tests for CConnman and PeerManager by vasild

maflcko referenced this in commit 9887fc7898 on Jan 11, 2023

sidhujag referenced this in commit 3b0597af60 on Jan 11, 2023

DrahtBot added the label Needs rebase on Jan 11, 2023

hebasto force-pushed on Jan 12, 2023

hebasto commented at 3:18 PM on January 12, 2023: member

Rebased ecb0262a5a45f545a18c93e42317e013b8740fba -> 1f1b3fdc1872ae30127de363fc8c72f1649626de (pr26642.03 -> pr26642.04) on top of the merged #26758.

DrahtBot removed the label Needs rebase on Jan 12, 2023

maflcko commented at 12:28 PM on January 17, 2023: member

CI fails. Also, the first commit could be split up?

hebasto force-pushed on Jan 17, 2023

hebasto cross-referenced this on Jan 17, 2023 from issue refactor: Remove duplication of `clang-tidy`'s check names by hebasto

hebasto commented at 1:43 PM on January 17, 2023: member

Also, the first commit could be split up?

Done in #26905.

hebasto commented at 1:49 PM on January 17, 2023: member

Rebased 1f1b3fdc1872ae30127de363fc8c72f1649626de -> 38f8938868f8ebebf3353e137957358399704a4b (pr26642.04 -> pr26642.05) on top of the merged bitcoin-core/gui#686.

CI fails.

Should be green now :)

maflcko referenced this in commit f41252f19d on Jan 17, 2023

hebasto force-pushed on Jan 17, 2023

hebasto commented at 3:55 PM on January 17, 2023: member

Rebased 38f8938868f8ebebf3353e137957358399704a4b -> 9e81656e92c91d128db8a5fcaecb0c385bb012cb (pr26642.05 -> pr26642.06) on top of the merged #26905.

sidhujag referenced this in commit 8d0c3a0140 on Jan 17, 2023

DrahtBot cross-referenced this on Jan 21, 2023 from issue refactor: Fix clang-tidy readability-const-return-type violations by maflcko

DrahtBot added the label Needs rebase on Feb 1, 2023

hebasto force-pushed on Feb 1, 2023

hebasto commented at 6:59 PM on February 1, 2023: member

Rebased 9e81656e92c91d128db8a5fcaecb0c385bb012cb -> 3f4e43caefb53ea726af07fb0802512264f82ae3 (pr26642.06 -> pr26642.07) on top of the merged #26705 and #26935. @martinus

Mind reviewing changes in bench/nanobench.h?

DrahtBot removed the label Needs rebase on Feb 1, 2023

martinus commented at 7:32 AM on February 2, 2023: contributor

Mind reviewing changes in bench/nanobench.h?

ACK the nanobench.h changes. I'd like to keep the version in sync with the upstream repository at https://github.com/martinus/nanobench

So I'll have a look at the clang-tidy checks there, do a bunch of updates, and then create a PR to update the nanobench version here to stay in sync. That might take a while though.

martinus cross-referenced this on Feb 3, 2023 from issue Update nanobench to version v4.3.10 by martinus

martinus commented at 6:16 AM on February 3, 2023: contributor

I created #27030 which compiles without warnings with the .clang-tidy changes here

hebasto commented at 8:54 AM on February 3, 2023: member

I created #27030 which compiles without warnings with the .clang-tidy changes here

Converting this PR to a draft for now.

hebasto marked this as a draft on Feb 3, 2023

fanquake referenced this in commit d8f9826037 on Feb 5, 2023

DrahtBot added the label Needs rebase on Feb 5, 2023

sidhujag referenced this in commit 965f71901e on Feb 5, 2023

hebasto force-pushed on Feb 6, 2023

hebasto marked this as ready for review on Feb 6, 2023

hebasto commented at 10:59 AM on February 6, 2023: member

I created #27030 which compiles without warnings with the .clang-tidy changes here

Converting this PR to a draft for now.

Rebased on the merged #27030, and is ready for a (final?) review :)

DrahtBot removed the label Needs rebase on Feb 6, 2023

in src/qt/walletmodel.h:126 in 79bf399e59 outdated

 122 | @@ -123,8 +123,8 @@ class WalletModel : public QObject
 123 |          // Copy constructor is disabled.
 124 |          UnlockContext(const UnlockContext&) = delete;
 125 |          // Move operator and constructor transfer the context
 126 | -        UnlockContext(UnlockContext&& obj) { CopyFrom(std::move(obj)); }
 127 | -        UnlockContext& operator=(UnlockContext&& rhs) { CopyFrom(std::move(rhs)); return *this; }
 128 | +        UnlockContext(UnlockContext&& obj) noexcept { CopyFrom(std::move(obj)); }

martinus commented at 12:36 PM on February 6, 2023:

I'm not sure this is correct. CopyFrom is not noexcept, so marking this as noexcept can be dangerous. CopyFrom seems to use the copy assignment operator with its *this = rhs; which too is not noexcept. I think it can currently be marked as noexcept (based on the members here), but this is dangerous; e.g. when you add any data that needs allocating, the noexcept would be a lie and bad_alloc exceptions would cause the program to abort

hebasto commented at 8:58 AM on February 16, 2023:

@martinus

The commented code is no longer relevant since https://github.com/bitcoin-core/gui/pull/711.

Nevertheless, I've dropped performance-noexcept-move-constructor commit for a follow up as it requires more thorough reviewing for similar cases.

martinus commented at 6:00 PM on February 16, 2023:

Good call, I find the noexcept stuff so brittle it's really hard to use. E.g. my changes to nanobench.h too were not completely correct because there's not even a guarantee that std::string move assignment is noexcept, see here: https://github.com/martinus/nanobench/pull/87

hebasto commented at 6:01 PM on February 14, 2023: member

Drafted. Please review https://github.com/bitcoin-core/gui/pull/711 first.

hebasto marked this as a draft on Feb 14, 2023

DrahtBot cross-referenced this on Feb 15, 2023 from issue refactor: Disable unused special members functions in `UnlockContext` by hebasto

DrahtBot added the label Needs rebase on Feb 16, 2023

hebasto marked this as ready for review on Feb 16, 2023

hebasto force-pushed on Feb 16, 2023

DrahtBot removed the label Needs rebase on Feb 16, 2023

hebasto commented at 8:59 AM on February 16, 2023: member

Ready for a (final?) review now :)

in src/test/scheduler_tests.cpp:74 in 2cdb84ca4c outdated

  70 | @@ -71,6 +71,7 @@ BOOST_AUTO_TEST_CASE(manythreads)
  71 |  
  72 |      // As soon as these are created they will start running and servicing the queue
  73 |      std::vector<std::thread> microThreads;
  74 | +    microThreads.reserve(5);

martinus commented at 6:36 PM on February 16, 2023:

nit: could be 10, 5 more are added :)

hebasto commented at 2:58 PM on February 22, 2023:

Thanks! Fixed.

in src/test/script_p2sh_tests.cpp:279 in 2cdb84ca4c outdated

 272 |      for (int i = 0; i < 6; i++)
 273 |      {
 274 |          key[i].MakeNewKey(true);
 275 |          BOOST_CHECK(keystore.AddKey(key[i]));
 276 |      }
 277 | +    std::vector<CPubKey> keys;

martinus commented at 6:43 PM on February 16, 2023:

In BOOST_AUTO_TEST_CASE(set) there's another std::vector<CPubKey> keys; without reserve, I wonder why clang-tidy doesn't see these

hebasto commented at 2:58 PM on February 22, 2023:

I wonder why clang-tidy doesn't see these

So do I :)

hebasto commented at 3:17 PM on March 23, 2023:

Updated.

martinus commented at 7:03 PM on February 16, 2023: contributor

I ran clang-tidy on this PR, and found a few more warnings:

/home/martinus/git/github.com/martinus/bitcoin/src/rpc/server.cpp:88:9: error: 'push_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
        vCommands.push_back(make_pair(entry.second.front()->category + entry.first, entry.second.front()));
        ^

/home/martinus/git/github.com/martinus/bitcoin/src/rpc/server.cpp:516:39: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
    for (const auto& i : mapCommands) commandList.emplace_back(i.first);
                                      ^

/home/martinus/git/github.com/martinus/bitcoin/src/rpc/util.cpp:612:9: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
        ret.emplace_back(arg.m_names);

hebasto force-pushed on Feb 22, 2023

hebasto commented at 2:57 PM on February 22, 2023: member

Updated 2cdb84ca4c96cef03946b14ec46a7720f3ece098 -> 7db868b49b60de371d01cda1f8e5faaa0e18f822 (pr26642.09 -> pr26642.10):

rebased
addressed @martinus's #26642 (review)

@martinus wrote:

I ran clang-tidy on this PR, and found a few more warnings

Is there a way to reproduce them in the CI environment?

martinus commented at 3:12 PM on February 22, 2023: contributor

I don't know much about how the CI works, maybe it's possible to install a newer clang version? I'm using 15.0.7

hebasto commented at 3:28 PM on February 22, 2023: member

I don't know much about how the CI works, maybe it's possible to install a newer clang version? I'm using 15.0.7

Ah, that makes sense.

Maybe keep this PR diff CI-driven? (I mean, the CI task passes now)

I assume that bumping tools versions in a CI task will force us to fix more warnings, not just those you have already mentioned.

fanquake commented at 9:54 AM on February 23, 2023: member

Maybe keep this PR diff CI-driven? (I mean, the CI task passes now)

If there are additional "fixes" being pointed out, which are correct, and based on the same rules that we are trying to apply, I don't really see the point of leaving those changes out, just to have to fix them later (upgrading our tooling here is another point).

I've been a little bit concerned that a lot of these clang-tidy changes have just been blindly following the tool/CI outout, without much thought as to wether changes are actually good/bad/complete or not, and are just being done for the sake of enabling things.

TheCharlatan approved

TheCharlatan commented at 10:34 AM on March 6, 2023: contributor

ACK 7db868b49b60de371d01cda1f8e5faaa0e18f822

I reproduced all changes here by running clang-tidy locally. They all seem like a clear improvement to me.

hebasto cross-referenced this on Mar 23, 2023 from issue ci: Use clang-15 in "tidy" task by hebasto

hebasto commented at 9:08 AM on March 23, 2023: member

I don't know much about how the CI works, maybe it's possible to install a newer clang version? I'm using 15.0.7

Done in #27311.

Making this PR a draft for now.

DrahtBot requested review from aureleoules on Mar 23, 2023

hebasto marked this as a draft on Mar 23, 2023

fanquake referenced this in commit f380bb93e8 on Mar 23, 2023

hebasto force-pushed on Mar 23, 2023

hebasto marked this as ready for review on Mar 23, 2023

hebasto commented at 3:17 PM on March 23, 2023: member

Rebased on top of the merged #27311.

All @martinus's comments have been addressed.

hebasto marked this as a draft on Mar 23, 2023

fanquake commented at 3:27 PM on March 23, 2023: member

Seeing lots of https://cirrus-ci.com/task/6739422046060544?logs=ci#L339:

/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/rpc/server.cpp:88:9: error: 'push_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
        vCommands.push_back(make_pair(entry.second.front()->category + entry.first, entry.second.front()));
        ^
/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/rpc/server.cpp:516:39: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-i617 warnings generated.

hebasto force-pushed on Mar 23, 2023

hebasto marked this as ready for review on Mar 23, 2023

hebasto commented at 3:44 PM on March 23, 2023: member

Seeing lots of https://cirrus-ci.com/task/6739422046060544?logs=ci#L339:

Sorry. Pushed a wrong branch.

Should be OK now.

TheCharlatan commented at 4:49 PM on March 23, 2023: contributor

re-ACK 09383b6

Locally, I'm also getting a bunch of (though still on clang-14):

error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]

after running:

bear --config src/.bear-tidy-config -- make -j $(nproc)
cd ./src/ && run-clang-tidy  -j $(nproc)

DrahtBot cross-referenced this on Mar 24, 2023 from issue RPC: Accept options as named-only parameters by ryanofsky

hebasto cross-referenced this on Mar 26, 2023 from issue util: implement `noexcept` move assignment & move ctor for `prevector` by martinus

in src/wallet/scriptpubkeyman.cpp:2505 in 09383b6122 outdated

2501 | @@ -2501,6 +2502,7 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction&
2502 |          } else {
2503 |              // Maybe there are pubkeys listed that we can sign for
2504 |              std::vector<CPubKey> pubkeys;
2505 | +            pubkeys.reserve(input.hd_keypaths.size());

martinus commented at 4:50 PM on March 26, 2023:

nit: maybe add + 2 to the reserve because when the next if is true 2 more will be added. Otherwise the next emplace_back will most likely cause a capacity change

hebasto commented at 7:22 PM on March 26, 2023:

Thanks! Updated.

martinus commented at 4:53 PM on March 26, 2023: contributor

code review ACK 09383b612216b63f2aec0f1fffc889989c66f212

clang-tidy: Add `performance-faster-string-find` check

https://clang.llvm.org/extra/clang-tidy/checks/performance/faster-string-find.html

516b75f66e

clang-tidy: Add `performance-inefficient-vector-operation` check

https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html

7e975e6cf8

clang-tidy: Add `performance-type-promotion-in-math-fn` check

https://clang.llvm.org/extra/clang-tidy/checks/performance/type-promotion-in-math-fn.html

2400437230

clang-tidy: Exclude `performance-*` checks rather including them 03ec5b6f9c

hebasto force-pushed on Mar 26, 2023

hebasto commented at 7:22 PM on March 26, 2023: member

Updated 09383b612216b63f2aec0f1fffc889989c66f212 -> 03ec5b6f9ca3af28c9ce25cf2393e28ae852d808 (pr26642.12 -> pr26642.13, diff):

addressed @martinus's comment

martinus commented at 5:46 AM on March 27, 2023: contributor

ACK 03ec5b6f9ca3af28c9ce25cf2393e28ae852d808

DrahtBot requested review from TheCharlatan on Mar 27, 2023

TheCharlatan commented at 6:08 AM on March 27, 2023: contributor

re-ACK 03ec5b6

DrahtBot removed review request from TheCharlatan on Mar 27, 2023

fanquake merged this on Mar 27, 2023

fanquake closed this on Mar 27, 2023

hebasto deleted the branch on Mar 27, 2023

sidhujag referenced this in commit d060ef7938 on Mar 27, 2023

bitcoin locked this on Mar 26, 2024