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

hebasto commented at 3:33 pm on December 15, 2022: member

This PR:

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

Closes bitcoin/bitcoin#26703.

DrahtBot commented at 3:33 pm on December 15, 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	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 commented at 3:44 pm on December 15, 2022: member

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 commented at 4:00 pm on December 15, 2022: member

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

RandyMcMillan commented at 5:11 pm on December 15, 2022: contributor

Concept ACK

I agree with @MarcoFalke

w0xlt approved

w0xlt commented at 6:17 pm on December 15, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26705/commits/fac1a47c203bb1351b344038443d72b8de4f14e2

hebasto commented at 9:39 pm on December 15, 2022: member

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

Please review:

Drafting this PR for now.

hebasto marked this as a draft on Dec 15, 2022

maflcko referenced this in commit 5055d07edf on Dec 16, 2022

DrahtBot added the label Needs rebase on Dec 16, 2022

hebasto force-pushed on Dec 16, 2022

DrahtBot removed the label Needs rebase on Dec 16, 2022

sidhujag referenced this in commit 5bcd427ef5 on Dec 16, 2022

maflcko referenced this in commit 6c01323d9d on Dec 17, 2022

DrahtBot added the label Needs rebase on Dec 17, 2022

hebasto force-pushed on Dec 17, 2022

hebasto commented at 11:40 am on December 17, 2022: member

it might be good to create one pull/commit for each bugfix.

The next one is #26710 :)

maflcko referenced this in commit cb32328d1b on Dec 17, 2022

hebasto force-pushed on Dec 17, 2022

DrahtBot removed the label Needs rebase on Dec 17, 2022

sidhujag referenced this in commit fd665e9f18 on Dec 17, 2022

sidhujag referenced this in commit 9f664d719e on Dec 17, 2022

DrahtBot added the label Needs rebase on Jan 16, 2023

hebasto referenced this in commit b7f6a89a3e on Jan 17, 2023

hebasto force-pushed on Jan 17, 2023

hebasto commented at 11:59 am on January 17, 2023: member

it might be easier to review if there was a separate pull/commit to fix broken std::move.

Please review #26707 first.

DrahtBot removed the label Needs rebase on Jan 17, 2023

hebasto force-pushed on Jan 17, 2023

DrahtBot added the label Needs rebase on Jan 23, 2023

maflcko referenced this in commit 30f553d457 on Jan 24, 2023

sidhujag referenced this in commit db74fb68cc on Jan 24, 2023

hebasto force-pushed on Jan 24, 2023

hebasto marked this as ready for review on Jan 24, 2023

hebasto commented at 8:59 pm on January 24, 2023: member

Rebased and ready for reviewing.

DrahtBot removed the label Needs rebase on Jan 24, 2023

in src/node/utxo_snapshot.h:16 in 4b9a21f7dd outdated

12@@ -13,7 +13,7 @@
13 
14 #include <optional>
15 
16-extern RecursiveMutex cs_main;
17+extern RecursiveMutex cs_main; // NOLINT(readability-redundant-declaration)

maflcko commented at 8:36 am on January 25, 2023:

https://github.com/bitcoin/bitcoin/pull/26965

hebasto commented at 11:34 am on January 30, 2023:

#26965

Yeah, it would be nice if #26965 go first.

DrahtBot added the label Needs rebase on Jan 26, 2023

hebasto force-pushed on Jan 26, 2023

maflcko commented at 4:21 pm on January 26, 2023: member

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

Edit: Modulo #26705 (review)

hebasto commented at 4:40 pm on January 26, 2023: member

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

Edit: Modulo #26705 (comment)

That’s true.

DrahtBot removed the label Needs rebase on Jan 26, 2023

DrahtBot added the label Needs rebase on Jan 30, 2023

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

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

96ee992ac3

clang-tidy: Force to check all headers b0e916913c

hebasto force-pushed on Jan 31, 2023

hebasto renamed this:
~~clang-tidy: Check headers and fixes them~~
clang-tidy: Fix `modernize-use-default-member-init` in headers and force to check all headers
on Jan 31, 2023

hebasto commented at 11:54 am on January 31, 2023: member

Rebased. The PR description has been updated.

DrahtBot removed the label Needs rebase on Jan 31, 2023

maflcko approved

maflcko commented at 9:35 am on February 1, 2023: member

review ACK b0e916913cedb8154419ec818bb9094a72fc8379 🍹

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK b0e916913cedb8154419ec818bb9094a72fc8379 🍹
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgbsgwAulUHjNHcytUabJBoaqpKSZ+tuqsd7gFyApI1CC7025pswCu+Ph1iCICt
 8rpOLfGLvKaWkDF54XUANxTSmk4xP9hnfYdRgllcVi7zbRg1I3Tnr3J5zt2L1PYQH
 9AcZ+p1fIZautxpg1440WHwlbJL3F8e84rQ2WPRJlpFt5gN9LuKKMVVvZpALna669
10kMVk2G9995kbpzDzQWPXYl0LC5aDwdeOIui+DUcHKzIYywTpMAHCMk1F4hUF1J6Q
11JdJ/X2pjNpxgRhCL2rdzZ47pZMUWfEMLMs0BslbCmg/6OdKa1NrtHCQZ1FOy+T3v
12/Y9D+Z1L7Ku9tzQPzADOy8oL410DZtrchANrlUVBoLBz34uWMHgp/jNqvqvy1jzh
132+4bNe+K8srE1E3y5mUvY68ddB6+4rE05mmKyBd9Rycg5XNsKVAsoZasbD0jBX9C
14adldbvwYXjOlIxXlllGGDrOB5m7kqHmWTaCN+SzjRyR/1Gy+a6epas/WHkiYHyuC
15HkGz+8hB
16=QqPj
17-----END PGP SIGNATURE-----

in src/span.h:110 in b0e916913c

106@@ -107,7 +107,7 @@ class Span
107 
108 
109 public:
110-    constexpr Span() noexcept : m_data(nullptr), m_size(0) {}
111+    constexpr Span() noexcept : m_data(nullptr) {}

maflcko commented at 9:38 am on February 1, 2023:

Any reason why nullptr isn’t fixed as well?

hebasto commented at 12:33 pm on February 1, 2023:

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

maflcko commented at 2:47 pm on February 1, 2023:

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

maflcko commented at 10:27 am on March 6, 2023:

Fixed in clang-tidy-17, see https://github.com/llvm/llvm-project/commit/fa491fefb0f862b653b196eb61f0e690b0b71460

maflcko merged this on Feb 1, 2023

maflcko closed this on Feb 1, 2023

hebasto deleted the branch on Feb 1, 2023

sidhujag referenced this in commit 28e7ecec8d on Feb 1, 2023

bitcoin locked this on Mar 5, 2024

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

Reviews

Conflicts

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