8000 clang-tidy: Add more `performance-*` checks and related fixes by hebasto · Pull Request #26642 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Mar 27, 2023
Merged

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

merged 4 commits into from
Mar 27, 2023

Conversation

hebasto
Copy link
Member
@hebasto hebasto commented Dec 5, 2022

No description provided.

@DrahtBot
Copy link
Contributor
DrahtBot commented Dec 5, 2022 •
edited
Loading
8000

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
Copy link
Contributor

Concept ACK

aureleoules
aureleoules approved these changes Dec 5, 2022
View reviewed changes
Copy link
Contributor
@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 66b5bdd

src/.clang-tidy Outdated
Comment on lines 9 to 12
-performance-no-int-to-ptr,
-performance-unnecessary-value-param,
Copy link
Contributor
@aureleoules aureleoules Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why these two are not enabled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The required fixes seem too invasive for this PR.

This was referenced Dec 6, 2022
Merged
Merged
Closed
Merged
maflcko
maflcko reviewed Dec 7, 2022
View reviewed changes
src/.clang-tidy Outdated
@@ -7,6 +7,7 @@ modernize-use-default-member-init,
modernize-use-nullptr,
performance-faster-string-find,
performance-for-range-copy,
performance-inefficient-string-concatenation,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Updated.

@hebasto
Copy link
Member Author
hebasto commented Dec 7, 2022

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

This was referenced Dec 13, 2022
Merged
Closed
maflcko
maflcko reviewed Dec 21, 2022
View reviewed changes
@DrahtBot DrahtBot mentioned this pull request Dec 22, 2022
Closed
@hebasto hebasto mentioned this pull request Dec 27, 2022
Closed
@DrahtBot DrahtBot mentioned this pull request Jan 4, 2023
Open
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Jan 11, 2023
Merge bitcoin/bitcoin#26758: refactor: Add `performance-no-automatic-…
9887fc7 
…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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 11, 2023
Merge bitcoin#26758: refactor: Add performance-no-automatic-move cl…
3b0597a 
…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
Copy link
Member Author
hebasto commented Jan 12, 2023

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

@maflcko
Copy link
Member
maflcko commented Jan 17, 2023

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

@hebasto hebasto mentioned this pull request Jan 17, 2023
Merged
@hebasto
Copy link
Member Author
hebasto commented Jan 17, 2023

Also, the first commit could be split up?

Done in #26905.

@hebasto
Copy link
Member Author
hebasto commented Jan 17, 2023

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

CI fails.

Should be green now :)

8000

maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Jan 17, 2023
Merge bitcoin/bitcoin#26905: refactor: Remove duplication of `clang-t…
f41252f 
…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
Copy link
Member Author
hebasto commented Mar 23, 2023

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 DrahtBot requested a review from aureleoules March 23, 2023 09:09
@hebasto hebasto marked this pull request as draft March 23, 2023 09:09
fanquake added a commit to bitcoin-core/gui that referenced this pull request Mar 23, 2023
@fanquake
Merge bitcoin/bitcoin#27311: ci: Use clang-15 in "tidy" task
f380bb9 
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 hebasto marked this pull request as ready for review March 23, 2023 15:15
@hebasto
Copy link
Member Author
hebasto commented Mar 23, 2023

Rebased on top of the merged #27311.

All @martinus's comments have been addressed.

@hebasto hebasto marked this pull request as draft March 23, 2023 15:27
@fanquake
Copy link
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 hebasto marked this pull request as ready for review March 23, 2023 15:31
@hebasto
Copy link
Member Author
hebasto commented Mar 23, 2023

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

Sorry. Pushed a wrong branch.

Should be OK now.

@TheCharlatan
Copy link
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 DrahtBot mentioned this pull request Mar 24, 2023
Merged
@hebasto hebasto mentioned this pull request Mar 26, 2023
Merged
martinus
martinus reviewed Mar 26, 2023
View reviewed changes
Copy link
Contributor
@martinus martinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review ACK 09383b6

@hebasto
Copy link
Member Author
hebasto commented Mar 26, 2023

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

@martinus
Copy link
Contributor

ACK 03ec5b6

@DrahtBot DrahtBot requested a review from TheCharlatan March 27, 2023 05:46
@TheCharlatan
Copy link
Contributor

re-ACK 03ec5b6
D73D

@DrahtBot DrahtBot removed the request for review from TheCharlatan March 27, 2023 06:08
@fanquake fanquake merged commit 3963067 into bitcoin:master Mar 27, 2023
@hebasto hebasto deleted the 221205-ci-tidy branch March 27, 2023 13:56
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 27, 2023
@fanquake
Merge bitcoin#26642: clang-tidy: Add more performance-* checks and …
d060ef7 
…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
@bitcoin bitcoin locked and limited conversation to collaborators Mar 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@martinus martinus martinus left review comments

@maflcko maflcko maflcko left review comments

@TheCharlatan TheCharlatan TheCharlatan approved these changes

@aureleoules aureleoules Awaiting requested review from aureleoules

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hebasto @DrahtBot @aureleoules @maflcko @martinus @fanquake @TheCharlatan
0