ci: Update Clang in "tidy" job

hebasto commented at 4:13 pm on September 20, 2025: member

This PR:

Updates to IWYU 0.25, which is compatible with Clang 21.
Fixes new “modernize-use-default-member-init” warnings. The warning in interpreter.cpp is a false positive, so it has been suppressed.

hebasto added the label Tests on Sep 20, 2025

DrahtBot commented at 4:13 pm on September 20, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33445.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, ryanofsky

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

hebasto commented at 4:17 pm on September 20, 2025: member

@ryanofsky

Could you please take a look at the following IPC-related warning:

0/usr/include/kj/async-inl.h:404:12: warning: Out of bound access to memory preceding the region [clang-analyzer-security.ArrayBound]
1  404 |       ctor(*ptr, kj::mv(next), kj::fwd<Params>(params)...);
2      |            ^

in src/qt/recentrequeststablemodel.h:21 in ecacfac7ea outdated

17@@ -18,10 +18,10 @@ class WalletModel;
18 class RecentRequestEntry
19 {
20 public:
21-    RecentRequestEntry() : nVersion(RecentRequestEntry::CURRENT_VERSION) {}
22+    RecentRequestEntry() = default;

vasild commented at 10:31 am on September 23, 2025:

Correct me if I am wrong, in the absence of other constructors (like in RecentRequestEntry), this line is not needed because it will generate the same constructor that will be generated if we don’t specify any constructor. In other words, I suggest to drop the line RecentRequestEntry() = default;.

hebasto commented at 12:31 pm on September 23, 2025:

Your analysis seems correct to me. However, I prefer being explicit here. Otherwise, if any user-declared constructor is added in the future, the default constructor would not be generated.

in src/qt/transactionfilterproxy.cpp:16 in ecacfac7ea

12@@ -13,8 +13,7 @@
13 
14 TransactionFilterProxy::TransactionFilterProxy(QObject* parent)
15     : QSortFilterProxyModel(parent),
16-      m_search_string(),
17-      typeFilter(ALL_TYPES)
18+      m_search_string()

vasild commented at 10:33 am on September 23, 2025:

My understanding is that such default constructing is not needed because m_search_string will be default constructed anyway. That is, drop the line m_search_string().

hebasto commented at 12:49 pm on September 23, 2025:

Thanks! Taken.

in src/zmq/zmqabstractnotifier.h:24 in ecacfac7ea

20@@ -21,7 +21,7 @@ class CZMQAbstractNotifier
21 public:
22     static const int DEFAULT_ZMQ_SNDHWM {1000};
23 
24-    CZMQAbstractNotifier() : outbound_message_high_water_mark(DEFAULT_ZMQ_SNDHWM) {}
25+    CZMQAbstractNotifier() = default;

vasild commented at 10:34 am on September 23, 2025:

Same here, remove CZMQAbstractNotifier() = default;?

hebasto commented at 12:49 pm on September 23, 2025:

Thanks! Fixed.

vasild approved

vasild commented at 10:35 am on September 23, 2025: contributor

ACK ecacfac7ead178ace3b0ec9393a7c63b5d8d3576

hebasto force-pushed on Sep 23, 2025

hebasto commented at 12:50 pm on September 23, 2025: member

The feedback from @vasild has been addressed.

vasild approved

vasild commented at 1:44 pm on September 23, 2025: contributor

ACK 1bb0f7f8b6d927046fefa2eb29d0132713415cca

ryanofsky referenced this in commit 67352719aa on Sep 23, 2025

in src/script/interpreter.cpp:1261 in 1bb0f7f8b6 outdated

1252@@ -1253,8 +1253,10 @@ class CTransactionSignatureSerializer
1253     const CScript& scriptCode; //!< output script being consumed
1254     const unsigned int nIn;    //!< input index of txTo being signed
1255     const bool fAnyoneCanPay;  //!< whether the hashtype has the SIGHASH_ANYONECANPAY flag set
1256+    // NOLINTBEGIN(modernize-use-default-member-init)
1257     const bool fHashSingle;    //!< whether the hashtype is SIGHASH_SINGLE
1258     const bool fHashNone;      //!< whether the hashtype is SIGHASH_NONE
1259+    // NOLINTEND(modernize-use-default-member-init)

ryanofsky commented at 3:19 pm on September 23, 2025:

In commit “ci: Update Clang in “tidy” job” (1bb0f7f8b6d927046fefa2eb29d0132713415cca)

I’m confused about why these are necessary. What makes these two members different from the fAnyoneCanPay member and causes clang-tidy to warn about them but not fAnyoneCanPay? All three members seem to be set in the same way by the same nHashTypeIn constructor argument.

This seems more like a bug that should be reported upstream than a change that should be made to our code. And if the change is necessary it would seem good to have a comment saying it is a temporary workaround.

hebasto commented at 9:10 pm on September 23, 2025:

This seems more like a bug that should be reported upstream…

https://github.com/llvm/llvm-project/issues/160394

hebasto commented at 9:15 pm on September 23, 2025:

… it would seem good to have a comment saying it is a temporary workaround.

Thanks! Done.

ryanofsky approved

ryanofsky commented at 3:23 pm on September 23, 2025: contributor

Code review ACK 1bb0f7f8b6d927046fefa2eb29d0132713415cca. Nice code cleanups, though did leave one comment about the NOLINT.

re: #33445 (comment)

Could you please take a look at the following IPC-related warning:

Thanks, created https://github.com/capnproto/capnproto/pull/2417 to address it

hebasto commented at 5:49 pm on September 23, 2025: member

re: #33445 (comment)

Could you please take a look at the following IPC-related warning:

Thanks, created capnproto/capnproto#2417 to address it

I’ve added a commit to temporarily silence this warning.

ci: Update Clang in "tidy" job

This change updates to IWYU 0.25, which is compatible with Clang 21.
Fixes new "modernize-use-default-member-init" warnings.
The warning in `interpreter.cpp` is a false positive, so it has been
suppressed.

9664135d17

clang-tidy: Disable `ArrayBound` check in src/ipc and src/test 5b20d172ca

hebasto force-pushed on Sep 23, 2025

hebasto commented at 9:16 pm on September 23, 2025: member

The feedback from @ryanofsky has been addressed.

vasild approved

vasild commented at 6:35 am on September 24, 2025: contributor

ACK 5b20d172ca2a46a2b525201b4ff2444f9d415d8c

DrahtBot requested review from ryanofsky on Sep 24, 2025

in src/test/CMakeLists.txt:181 in 5b20d172ca

176@@ -177,6 +177,8 @@ if(ENABLE_IPC)
177       ipc_tests.cpp
178   )
179   target_link_libraries(test_bitcoin bitcoin_ipc_test bitcoin_ipc)
180+
181+  configure_file(.clang-tidy.in .clang-tidy USE_SOURCE_PERMISSIONS COPYONLY)

ryanofsky commented at 2:32 pm on September 25, 2025:

In commit “clang-tidy: Disable ArrayBound check in src/ipc and src/test” (5b20d172ca2a46a2b525201b4ff2444f9d415d8c)

Kind of a shame to need this outside of the IPC directory. Maybe there should be a src/ipc/test subdirectory like src/wallet/test to keep IPC stuff grouped together.

hebasto commented at 2:40 pm on September 25, 2025:

The same thought occurred to me while working on this PR, but I thought it might be better to defer it to a separate PR. Do you think it’s worth adding another commit to move the tests here?

ryanofsky commented at 2:57 pm on September 25, 2025:

re: #33445 (review)

The same thought occurred to me while working on this PR, but I thought it might be better to defer it to a separate PR. Do you think it’s worth adding another commit to move the tests here?

I was thinking of it as a followup for a future PR, but I’m actually not sure how big the change would be and whether it might require code changes. Maybe if it’s a small change it would be easy to make here. Whatever your preference is seems fine and I do think current solution makes sense given the current layout.

hebasto commented at 4:03 pm on September 25, 2025:

I was thinking of it as a followup for a future PR…

I agree with that.

… but I’m actually not sure how big the change would be and whether it might require code changes.

Something like a978c6bdb41fd3f8461908c30bb409a26fa62d47 in https://github.com/hebasto/bitcoin/commits/250925-llvm-tidy/.

ryanofsky approved

ryanofsky commented at 2:35 pm on September 25, 2025: contributor

Code review ACK 5b20d172ca2a46a2b525201b4ff2444f9d415d8c. Just added link to upstream modernize-use-default-member-init bug (thanks for looking into that and reporting) and added new suppressions for capnproto clang-tidy errors since last review

ci: Update Clang in “tidy” job #33445

Code Coverage & Benchmarks

Reviews