ci: Update Clang in "tidy" job #33445

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	ryanofsky, maflcko
Stale ACK	vasild

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:

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

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:1262 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.

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:184 in 5b20d172ca outdated

 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

fhanau referenced this in commit 2159bfd608 on Sep 29, 2025

fhanau referenced this in commit 76b044e04b on Sep 29, 2025

fhanau referenced this in commit df0f1cb485 on Sep 30, 2025

hebasto commented at 11:29 AM on October 6, 2025: member

Friendly ping @maflcko @fanquake :)

hebasto closed this on Oct 28, 2025

hebasto reopened this on Oct 28, 2025

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.

5efdb0ef30

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

in src/script/interpreter.cpp:1256 in 5b20d172ca

1252 | @@ -1254,8 +1253,12 @@ 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 | +    // Temporary workaround for a clang-tidy bug.

fanquake commented at 3:19 PM on October 28, 2025:

Given the upstream fix has landed (and was possibly backported?), this workaround should either be removed entirely, or updated to indicate the version of Clang which this was fixed in.

hebasto commented at 3:36 PM on October 28, 2025:

The comment has been updated.

and was possibly backported?

No, it wasn't.

hebasto force-pushed on Oct 28, 2025

hebasto commented at 3:36 PM on October 28, 2025: member

Rebased and addressed feedback from @fanquake.

hebasto commented at 3:30 PM on November 3, 2025: member

@vasild @ryanofsky

Kindly asking you to refresh your ACKs after the comment update.

ryanofsky approved

ryanofsky commented at 4:26 PM on November 3, 2025: contributor

Code review ACK 5d784bebaff5e3acc0b5180ee51d9a16aec0e356, just adding clang version comment since last review.

Would still be nice to remove ipc build code from the test directory as a followup too (https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379651340)

DrahtBot requested review from vasild on Nov 3, 2025

maflcko commented at 8:20 AM on November 4, 2025: member

review ACK 5d784bebaff5e3acc0b5180ee51d9a16aec0e356 🎒

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 5d784bebaff5e3acc0b5180ee51d9a16aec0e356 🎒
/Flat3dfMtdVCyisqRmXnrrqdQxlTYjyYTH6+6P04ON5gkskBepJWaDELcnfysyR58QGN+HTU8o8hccPj1YtDg==

</details>

fanquake merged this on Nov 4, 2025

fanquake closed this on Nov 4, 2025

hebasto deleted the branch on Nov 4, 2025