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

hebasto · 2022-12-05T18:40:46Z

No description provided.

DrahtBot · 2022-12-05T18:40:48Z

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 · 2022-12-05T19:39:02Z

Concept ACK

aureleoules

ACK 66b5bdd

aureleoules · 2022-12-05T22:32:49Z

src/.clang-tidy

+-performance-no-int-to-ptr,
+-performance-unnecessary-value-param,


Is there a reason why these two are not enabled?

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

The required fixes seem too invasive for this PR.

maflcko · 2022-12-07T15:24:21Z

src/.clang-tidy

 modernize-use-nullptr,
 performance-faster-string-find,
 performance-for-range-copy,
+performance-inefficient-string-concatenation,


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.

Thanks! Updated.

hebasto · 2022-12-07T17:09:49Z

Updated 66b5bdd -> 0524c30 (pr26642.01 -> pr26642.02):

addressed @MarcoFalke comment

src/.clang-tidy

…move` clang-tidy check 9567bfe clang-tidy: Add `performance-no-automatic-move` check (Hennadii Stepanov) Pull request description: Split from bitcoin/bitcoin#26642 as [requested](bitcoin/bitcoin#26642 (comment)). For the problem description see https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html. The following types are affected: - `std::pair<CAddress, NodeSeconds>` - `std::vector<CAddress>` - `UniValue`, also see bitcoin/bitcoin#25429 - `QColor` - `CBlock` - `MempoolAcceptResult` - `std::shared_ptr<CWallet>` - `std::optional<SelectionResult>` - `CTransactionRef`, which is `std::shared_ptr<const CTransaction>` ACKs for top commit: andrewtoth: ACK 9567bfe aureleoules: ACK 9567bfe Tree-SHA512: 9b6a5d539205b41d2c86402d384318ed2e1d89e66333ebd200a48fd7df3ce6f6c60a3e989eda5cc503fb34b8d82526f95e56776e1af51e63b49e3a1fef72dbcb

…ang-tidy check 9567bfe clang-tidy: Add `performance-no-automatic-move` check (Hennadii Stepanov) Pull request description: Split from bitcoin#26642 as [requested](bitcoin#26642 (comment)). For the problem description see https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html. The following types are affected: - `std::pair<CAddress, NodeSeconds>` - `std::vector<CAddress>` - `UniValue`, also see bitcoin#25429 - `QColor` - `CBlock` - `MempoolAcceptResult` - `std::shared_ptr<CWallet>` - `std::optional<SelectionResult>` - `CTransactionRef`, which is `std::shared_ptr<const CTransaction>` ACKs for top commit: andrewtoth: ACK 9567bfe aureleoules: ACK 9567bfe Tree-SHA512: 9b6a5d539205b41d2c86402d384318ed2e1d89e66333ebd200a48fd7df3ce6f6c60a3e989eda5cc503fb34b8d82526f95e56776e1af51e63b49e3a1fef72dbcb

hebasto · 2023-01-12T15:18:02Z

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

maflcko · 2023-01-17T12:28:22Z

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

hebasto · 2023-01-17T13:43:04Z

Also, the first commit could be split up?

Done in #26905.

hebasto · 2023-01-17T13:49:06Z

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

CI fails.

Should be green now :)

…idy`'s check names 06fc293 refactor: Remove duplication of clang-tidy's check names (Hennadii Stepanov) Pull request description: This PR removes duplication of `clang-tidy`'s check names. No behavior change. Split up from bitcoin/bitcoin#26642 as [requested](bitcoin/bitcoin#26642 (comment)). ACKs for top commit: fanquake: ACK 06fc293 Tree-SHA512: a21bef3d7d7201e14565b526af2eae7a90cf0f792803704a80a70a4c78f07ef2a2eef6a8dced80361efbf13291ecccb0977378b9532fc30970a2070426e4d82c

hebasto · 2023-03-23T09:08:57Z

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.

8fe27fb ci: Use clang-15 in "tidy" task (Hennadii Stepanov) Pull request description: Newer tools usually are better in terms of features and bug fixes. Requested in bitcoin/bitcoin#26642 (comment). Split from bitcoin/bitcoin#26766. ACKs for top commit: MarcoFalke: lgtm ACK 8fe27fb Tree-SHA512: 62be3307d488fc4f75c40c0fa095aaa091aade2d5fe85296b56751e006c801f9d58c72c5cee8c0a0b1ba1a43804e315a3301c03e6e394bb3f3eb9b763fbb6271

hebasto · 2023-03-23T15:17:00Z

Rebased on top of the merged #27311.

All @martinus's comments have been addressed.

fanquake · 2023-03-23T15:27:32Z

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 · 2023-03-23T15:44:35Z

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

Sorry. Pushed a wrong branch.

Should be OK now.

sedited · 2023-03-23T16:49:42Z

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)

martinus

code review ACK 09383b6

src/wallet/scriptpubkeyman.cpp

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

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

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

hebasto · 2023-03-26T19:22:17Z

Updated 09383b6 -> 03ec5b6 (pr26642.12 -> pr26642.13, diff):

addressed @martinus's comment

martinus · 2023-03-27T05:46:41Z

ACK 03ec5b6

sedited · 2023-03-27T06:08:16Z

re-ACK 03ec5b6

…related fixes 03ec5b6 clang-tidy: Exclude `performance-*` checks rather including them (Hennadii Stepanov) 2400437 clang-tidy: Add `performance-type-promotion-in-math-fn` check (Hennadii Stepanov) 7e975e6 clang-tidy: Add `performance-inefficient-vector-operation` check (Hennadii Stepanov) 516b75f clang-tidy: Add `performance-faster-string-find` check (Hennadii Stepanov) Pull request description: ACKs for top commit: martinus: ACK 03ec5b6 TheCharlatan: re-ACK [03ec5b6](bitcoin@03ec5b6) Tree-SHA512: 2dfa52f9131da88826f32583bfd534a56a998477db9804b7333c0e7ac0b6b36141009755c7163b9f95d0ecbf5c2cb63f8a69ce4b114bb83423faed21b50cec67

aureleoules approved these changes Dec 5, 2022

View reviewed changes

maflcko reviewed Dec 7, 2022

View reviewed changes

hebasto force-pushed the 221205-ci-tidy branch from 66b5bdd to 0524c30 Compare December 7, 2022 17:04

hebasto force-pushed the 221205-ci-tidy branch from 0524c30 to ecb0262 Compare December 7, 2022 17:25

This was referenced Dec 13, 2022

doc: Fix incorrect sendmany RPC doc #25093

Merged

[Draft / POC] Silent Payments #24897

Closed

maflcko reviewed Dec 21, 2022

View reviewed changes

src/.clang-tidy Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 22, 2022

refactor: Avoid UniValue copies #25429

Closed

hebasto mentioned this pull request Dec 27, 2022

refactor: Add performance-no-automatic-move clang-tidy check #26758

Closed

DrahtBot mentioned this pull request Jan 4, 2023

test: add end-to-end tests for CConnman and PeerManager #26812

Open

DrahtBot added the Needs rebase label Jan 11, 2023

hebasto force-pushed the 221205-ci-tidy branch from ecb0262 to 1f1b3fd Compare January 12, 2023 15:15

DrahtBot removed the Needs rebase label Jan 12, 2023

hebasto force-pushed the 221205-ci-tidy branch from 1f1b3fd to 38f8938 Compare January 17, 2023 13:38

hebasto mentioned this pull request Jan 17, 2023

refactor: Remove duplication of clang-tidy's check names #26905

Merged

hebasto force-pushed the 221205-ci-tidy branch from 38f8938 to 9e81656 Compare January 17, 2023 15:53

DrahtBot requested a review from aureleoules March 23, 2023 09:09

hebasto marked this pull request as draft March 23, 2023 09:09

hebasto force-pushed the 221205-ci-tidy branch from 7db868b to 38b0cbc Compare March 23, 2023 15:15

hebasto marked this pull request as ready for review March 23, 2023 15:15

hebasto marked this pull request as draft March 23, 2023 15:27

hebasto force-pushed the 221205-ci-tidy branch from 38b0cbc to 09383b6 Compare March 23, 2023 15:31

hebasto marked this pull request as ready for review March 23, 2023 15:31

DrahtBot mentioned this pull request Mar 24, 2023

RPC: Accept options as named-only parameters #26485

Merged

hebasto mentioned this pull request Mar 26, 2023

util: implement noexcept move assignment & move ctor for prevector #27334

Merged

martinus reviewed Mar 26, 2023

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

hebasto added 4 commits March 26, 2023 20:17

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

516b75f

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

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

7e975e6

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

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

2400437

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

clang-tidy: Exclude performance-* checks rather including them

03ec5b6

hebasto force-pushed the 221205-ci-tidy branch from 09383b6 to 03ec5b6 Compare March 26, 2023 19:18

DrahtBot requested a review from sedited March 27, 2023 05:46

DrahtBot removed the request for review from sedited March 27, 2023 06:08

fanquake merged commit 3963067 into bitcoin:master Mar 27, 2023

hebasto deleted the 221205-ci-tidy branch March 27, 2023 13:56

bitcoin locked and limited conversation to collaborators Mar 26, 2024

		-performance-no-int-to-ptr,
		-performance-unnecessary-value-param,

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

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

Uh oh!

Conversation

hebasto commented Dec 5, 2022

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

aureleoules commented Dec 5, 2022

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

aureleoules Dec 5, 2022 • 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!

hebasto Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 7, 2022

Uh oh!

Uh oh!

hebasto commented Jan 12, 2023

Uh oh!

maflcko commented Jan 17, 2023

Uh oh!

hebasto commented Jan 17, 2023

Uh oh!

hebasto commented Jan 17, 2023

Uh oh!

hebasto commented Mar 23, 2023

Uh oh!

hebasto commented Mar 23, 2023

Uh oh!

fanquake commented Mar 23, 2023

Uh oh!

hebasto commented Mar 23, 2023

Uh oh!

sedited commented Mar 23, 2023

Uh oh!

martinus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Mar 26, 2023

Uh oh!

martinus commented Mar 27, 2023

Uh oh!

sedited commented Mar 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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

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

DrahtBot commented Dec 5, 2022 •

edited

Loading

aureleoules Dec 5, 2022 •

edited

Loading