clang-tidy: Fix `modernize-use-default-member-init` in headers and force to check all headers #26705

hebasto · 2022-12-15T15:33:56Z

This PR:

fixes the only remained check in headers, i.e., modernize-use-default-member-init
forces clang-tidy check all headers

DrahtBot · 2022-12-15T15:33:59Z

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	MarcoFalke
Concept ACK	RandyMcMillan
Stale ACK	w0xlt

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:

#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)
#26312 (Remove Sock::Get() and Sock::Sock() by vasild)
#21878 (Make all networking code mockable by vasild)
#14485 (Try to use posix_fadvise with CBufferedFile by luke-jr)
#10443 (Add fee_est tool for debugging fee estimation code 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.

maflcko · 2022-12-15T15:44:05Z

concept ack, but it might be good to create one pull/commit for each bugfix. For example, it might be easier to review if there was a separate pull/commit to fix broken std::move.

aureleoules · 2022-12-15T16:00:01Z

Could this be done with a scripted-diff script using the clang-tidy -fix-errors command?

RandyMcMillan · 2022-12-15T17:11:49Z

Concept ACK

I agree with @MarcoFalke

w0xlt

ACK fac1a47

hebasto · 2022-12-15T21:39:36Z

concept ack, but it might be good to create one pull/commit for each bugfix.

Please review:

Drafting this PR for now.

…ring-init` in headers c39619e clang-tidy: Fix `readability-redundant-string-init` in headers (Hennadii Stepanov) Pull request description: Split from #26705 as was requested in #26705 (comment). To test this PR, consider applying a diff as follows: ```diff --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -12,17 +12,9 @@ readability-redundant-declaration, readability-redundant-string-init, ' WarningsAsErrors: ' -bugprone-argument-comment, -bugprone-use-after-move, -misc-unused-using-decls, -modernize-use-default-member-init, -modernize-use-nullptr, -performance-for-range-copy, -performance-move-const-arg, -performance-unnecessary-copy-initialization, -readability-redundant-declaration, readability-redundant-string-init, ' CheckOptions: - key: performance-move-const-arg.CheckTriviallyCopyableMove value: false +HeaderFilterRegex: '.' ``` ACKs for top commit: MarcoFalke: review ACK c39619e Tree-SHA512: d7b61be17737f68b8bb40b084cf03f89eae86f4951da2aa000fde0c5245491a01dbb83e5d6e870c6bab4de2dbb5c0eb0dd6613da71592b3a27cf2000a45eaeeb

…nings in headers 1308b83 clang-tidy: Fix `performance-no-automatic-move` in headers (Hennadii Stepanov) 0a5dc03 clang-tidy: Fix `performance-move-const-arg` in headers (Hennadii Stepanov) Pull request description: Split from bitcoin/bitcoin#26705 as was requested in bitcoin/bitcoin#26705 (comment). To test this PR, consider applying a diff as follows: ```diff --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -1,16 +1,7 @@ Checks: ' -*, -bugprone-argument-comment, -bugprone-use-after-move, -misc-unused-using-decls, -modernize-use-default-member-init, -modernize-use-nullptr, -performance-for-range-copy, performance-move-const-arg, performance-no-automatic-move, -performance-unnecessary-copy-initialization, -readability-redundant-declaration, -readability-redundant-string-init, ' WarningsAsErrors: ' bugprone-argument-comment, @@ -28,4 +19,4 @@ readability-redundant-string-init, CheckOptions: - key: performance-move-const-arg.CheckTriviallyCopyableMove value: false -HeaderFilterRegex: './qt' +HeaderFilterRegex: '.' ``` ACKs for top commit: fanquake: ACK 1308b83 Tree-SHA512: b7ef9a3e789846130ab4c3fd6fbe8d887bdbcd438e4cbc78e2b1ac01f819ae13d7f69c2a25f480bd36e3e7f58886a7d5a8609a3c3275c315e0697cd4010474bd

… headers 1308b83 clang-tidy: Fix `performance-no-automatic-move` in headers (Hennadii Stepanov) 0a5dc03 clang-tidy: Fix `performance-move-const-arg` in headers (Hennadii Stepanov) Pull request description: Split from bitcoin#26705 as was requested in bitcoin#26705 (comment). To test this PR, consider applying a diff as follows: ```diff --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -1,16 +1,7 @@ Checks: ' -*, -bugprone-argument-comment, -bugprone-use-after-move, -misc-unused-using-decls, -modernize-use-default-member-init, -modernize-use-nullptr, -performance-for-range-copy, performance-move-const-arg, performance-no-automatic-move, -performance-unnecessary-copy-initialization, -readability-redundant-declaration, -readability-redundant-string-init, ' WarningsAsErrors: ' bugprone-argument-comment, @@ -28,4 +19,4 @@ readability-redundant-string-init, CheckOptions: - key: performance-move-const-arg.CheckTriviallyCopyableMove value: false -HeaderFilterRegex: './qt' +HeaderFilterRegex: '.' ``` ACKs for top commit: fanquake: ACK 1308b83 Tree-SHA512: b7ef9a3e789846130ab4c3fd6fbe8d887bdbcd438e4cbc78e2b1ac01f819ae13d7f69c2a25f480bd36e3e7f58886a7d5a8609a3c3275c315e0697cd4010474bd

hebasto · 2023-01-24T20:59:00Z

Rebased and ready for reviewing.

src/node/utxo_snapshot.h

maflcko · 2023-01-26T16:21:55Z

So this is only fixing modernize-use-default-member-init, and no other remaining checks?

Edit: Modulo #26705 (comment)

hebasto · 2023-01-26T16:40:04Z

So this is only fixing modernize-use-default-member-init, and no other remaining checks?

Edit: Modulo #26705 (comment)

That's true.

See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html

hebasto · 2023-01-31T11:54:36Z

Rebased. The PR description has been updated.

maflcko

review ACK b0e9169 🍹

Show signature

Signature:

maflcko · 2023-02-01T09:38:28Z

src/span.h

@@ -107,7 +107,7 @@ class Span


 public:
-    constexpr Span() noexcept : m_data(nullptr), m_size(0) {}
+    constexpr Span() noexcept : m_data(nullptr) {}


Any reason why nullptr isn't fixed as well?

Yeah, it has been overlooked. The clang-tidy does not complain, though.

I guess it refrains because there are multiple constructors: https://github.com/llvm/llvm-project/blob/78f88082de3627ea04501c83a08f52cf1e60b4f7/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init.cpp#L56

Fixed in clang-tidy-17, see llvm/llvm-project@fa491fe

…t in …

w0xlt approved these changes Dec 15, 2022

View reviewed changes

This was referenced Dec 15, 2022

clang-tidy: Fix performance-*move* warnings in headers #26707

Merged

clang-tidy: Fix modernize-use-nullptr in headers #26708

Merged

clang-tidy: Fix readability-redundant-string-init in headers bitcoin-core/gui#685

Merged

hebasto marked this pull request as draft December 15, 2022 21:39

This was referenced Dec 16, 2022

Make all networking code mockable #21878

Closed

Try to use posix_fadvise with CBufferedFile #14485

Closed

DrahtBot added the Needs rebase label Dec 16, 2022

hebasto force-pushed the 221215-tidy branch from fac1a47 to 3d9e516 Compare December 16, 2022 10:24

This was referenced Dec 16, 2022

refactor: Fix performance-for-range-copy in headers #26710

Merged

clang-tidy: Force checks for headers in src/qt bitcoin-core/gui#686

Merged

DrahtBot removed the Needs rebase label Dec 16, 2022

DrahtBot mentioned this pull request Jan 21, 2023

Add fee_est tool for debugging fee estimation code #10443

Closed

DrahtBot added the Needs rebase label Jan 23, 2023

hebasto force-pushed the 221215-tidy branch from f7902f9 to 4b9a21f Compare January 24, 2023 20:44

hebasto marked this pull request as ready for review January 24, 2023 20:47

DrahtBot removed the Needs rebase label Jan 24, 2023

maflcko reviewed Jan 25, 2023

View reviewed changes

src/node/utxo_snapshot.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 25, 2023

refactor: Remove stray cs_main redundant declaration #26965

Merged

DrahtBot added the Needs rebase label Jan 26, 2023

hebasto force-pushed the 221215-tidy branch from 4b9a21f to c0d5b27 Compare January 26, 2023 16:07

DrahtBot added Needs rebase and removed Needs rebase labels Jan 26, 2023

hebasto added 2 commits January 31, 2023 11:50

clang-tidy: Fix modernize-use-default-member-init in headers

96ee992

See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html

clang-tidy: Force to check all headers

b0e9169

hebasto force-pushed the 221215-tidy branch from c0d5b27 to b0e9169 Compare January 31, 2023 11:50

hebasto changed the title ~~clang-tidy: Check headers and fixes them~~ clang-tidy: Fix modernize-use-default-member-init in headers and force to check all headers Jan 31, 2023

DrahtBot removed the Needs rebase label Jan 31, 2023

maflcko approved these changes Feb 1, 2023

View reviewed changes

maflcko reviewed Feb 1, 2023

View reviewed changes

maflcko merged commit e1bf547 into bitcoin:master Feb 1, 2023

hebasto deleted the 221215-tidy branch February 1, 2023 12:29

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 1, 2023

Merge bitcoin#26705: clang-tidy: Fix modernize-use-default-member-ini…

28e7ece

…t in …

hebasto mentioned this pull request Feb 1, 2023

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

Merged

hebasto mentioned this pull request Mar 21, 2023

refactor: Use move semantics instead of custom swap functions #26749

Merged

bitcoin locked and limited conversation to collaborators Mar 5, 2024

clang-tidy: Fix modernize-use-default-member-init in headers and force to check all headers #26705

clang-tidy: Fix modernize-use-default-member-init in headers and force to check all headers #26705

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko commented Dec 15, 2022

Uh oh!

aureleoules commented Dec 15, 2022

Uh oh!

RandyMcMillan commented Dec 15, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

hebasto commented Jan 24, 2023

Uh oh!

Uh oh!

maflcko commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 26, 2023

Uh oh!

hebasto commented Jan 31, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clang-tidy: Fix `modernize-use-default-member-init` in headers and force to check all headers #26705

clang-tidy: Fix `modernize-use-default-member-init` in headers and force to check all headers #26705

hebasto commented Dec 15, 2022 •

edited

Loading

DrahtBot commented Dec 15, 2022 •

edited

Loading

hebasto commented Dec 15, 2022 •

edited

Loading

maflcko commented Jan 26, 2023 •

edited

Loading