ci: Update Clang in "tidy" job

hebasto commented at 4:25 pm on November 17, 2024: member

This PR switches to the latest IWYU 0.23, which is compatible with Clang 19.

New “bugprone-use-after-move” and “modernize-use-starts-ends-with” warnings that emerged have been addressed.

hebasto added the label Refactoring on Nov 17, 2024

hebasto added the label Tests on Nov 17, 2024

DrahtBot commented at 4:25 pm on November 17, 2024: 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/31306.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, l0rinc, theuni

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:

#31308 (ci, iwyu: Treat warnings as errors for specific targets by hebasto)
#30997 (build: Switch to Qt 6 by hebasto)
#30975 (Add multiprocess binaries to release build by Sjors)

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.

hebasto force-pushed on Nov 17, 2024

in src/.clang-tidy:22 in 4b833038de outdated

18@@ -19,8 +19,10 @@ performance-*,
19 -performance-avoid-endl,
20 -performance-enum-size,
21 -performance-inefficient-string-concatenation,
22+-performance-inefficient-vector-operation,

l0rinc commented at 9:04 pm on November 17, 2024:

Given that #31305 fixes the issue, are we disabling it as a temporary measure?

hebasto commented at 9:07 pm on November 17, 2024:

Right.

It depends on which PR is merged first :)

in ci/test/00_setup_env_native_tidy.sh:9 in 4b833038de outdated

 5@@ -6,9 +6,9 @@
 6 
 7 export LC_ALL=C.UTF-8
 8 
 9-export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04"
10+export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.10"

maflcko commented at 8:39 am on November 18, 2024:

Not sure about using releases which are basically EOL in less than 6 months after the release branch is created. This requires touching the release branches to bump those along.

My preference would be to use APT_LLVM_V

hebasto commented at 11:37 am on November 25, 2024:

Thanks! Switched to use APT_LLVM_V.

in src/test/result_tests.cpp:66 in 4b833038de outdated

63+void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, const Args&... args)
64 {
65     ExpectResult(result, true, str);
66     BOOST_CHECK_EQUAL(result.has_value(), true);
67-    BOOST_CHECK_EQUAL(result.value(), T{std::forward<Args>(args)...});
68+    BOOST_CHECK_EQUAL(result.value(), T{args...});

maflcko commented at 8:44 am on November 18, 2024:

are you sure this is correct? Not that it matters, but isn’t it theoretically pessimising in ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S"));?

hebasto commented at 11:39 am on November 25, 2024:

This change has been removed from this PR.

in src/test/coins_tests.cpp:774 in 4532e1d509 outdated

770@@ -771,11 +771,10 @@ static void CheckAddCoinBase(CAmount base_value, CAmount cache_value, CAmount mo
771 // while still verifying that the CoinsViewCache::AddCoin implementation
772 // ignores base values.
773 template <typename... Args>
774-static void CheckAddCoin(Args&&... args)

l0rinc commented at 10:27 am on November 18, 2024:

This was already removed in the ACK-ed https://github.com/bitcoin/bitcoin/pull/30906/files#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33L774-L777, could you please either revert this line or do it in a separate PR, since I think removing it completely is a better fix.

hebasto commented at 11:39 am on November 25, 2024:

This change has been removed from this PR.

l0rinc commented at 12:37 pm on November 25, 2024:

Thank you!

l0rinc commented at 3:56 pm on November 27, 2024:

I’d like to revive my original concerns here: c4eb31b (#30906) removes this method completely, which I think is a better fix for it than the current workaround.

hebasto commented at 11:28 am on December 5, 2024:

#30906 has been merged, which makes this discussion outdated.

l0rinc commented at 12:05 pm on December 6, 2024:

Yes, thanks for waiting it out

in src/test/result_tests.cpp:62 in 4532e1d509 outdated

58@@ -59,12 +59,11 @@ void ExpectResult(const util::Result<T>& result, bool success, const bilingual_s
59 }
60 
61 template <typename T, typename... Args>
62-void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args&&... args)
63+void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, const Args&... args)

l0rinc commented at 10:35 am on November 18, 2024:

Looking at the actual usage, ExpectSuccess doesn’t need to be a variadic template function:

0ExpectSuccess(IntFn(5, true), {}, 5);
1ExpectSuccess(NoCopyFn(5, true), {}, 5);
2ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S"));

, we might as well simplify to something like:

0template <typename T, typename U>
1void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, U&& expected)
2{
3    ExpectResult(result, true, str);
4    BOOST_CHECK_EQUAL(result.has_value(), true);
5    BOOST_CHECK_EQUAL(result.value(), expected);
6    BOOST_CHECK_EQUAL(&result.value(), &*result);
7}

hebasto commented at 11:40 am on November 25, 2024:

This change has been removed from this PR.

maflcko commented at 8:04 am on November 28, 2024:

Looking at the actual usage, ExpectSuccess doesn’t need to be a variadic template function:

This may or may not be true right now, but the function has been intentionally written the way it is now. This is a pretty standard and common boilerplate code to pass a variable number of args to a constructor.

Changing it would be problematic, because it will need to be touched again in the future to be reverted, if there is a constructor with more than one arg.

Also, your suggestion will fail to compile if the constructor accepts a single arg and is marked explicit.

l0rinc changes_requested

l0rinc commented at 10:37 am on November 18, 2024: contributor

Concept ACK, but approach NACK, I think some of the examples can be fixed by removing the “feature” instead of just patching them.

in src/.clang-tidy:25 in 4b833038de outdated

18@@ -19,8 +19,10 @@ performance-*,
19 -performance-avoid-endl,
20 -performance-enum-size,
21 -performance-inefficient-string-concatenation,
22+-performance-inefficient-vector-operation,
23 -performance-no-int-to-ptr,
24 -performance-noexcept-move-constructor,
25+-performance-unnecessary-copy-initialization,

l0rinc commented at 6:39 pm on November 24, 2024:

Similarly to #31305, I’ve pushed a PR fixing this instead of disabling the check: https://github.com/bitcoin/bitcoin/pull/31364

hebasto force-pushed on Nov 25, 2024

hebasto commented at 11:41 am on November 25, 2024: member

@maflcko @l0rinc

Thank you for your reviews. The PR has been updated per your feedback.

l0rinc approved

l0rinc commented at 12:47 pm on November 25, 2024: contributor

ACK 3e039b7a03bea6e11e994aebe038ebd6ca80aeb8

The latest change:

Reverted the CI_IMAGE_NAME_TAG and added APT_LLVM_V.
Reverted the CheckAddCoin and ExpectSuccess fixes to avoid collisions with other PRs.
Updated description to reflect the dependent fix PRs.

in src/wallet/walletdb.cpp:1016 in 3c259019ff outdated

1012@@ -1013,7 +1013,7 @@ static DBErrors LoadAddressBookRecords(CWallet* pwallet, DatabaseBatch& batch) E
1013             // "1" or "p" for present (which was written prior to
1014             // f5ba424cd44619d9b9be88b8593d69a7ba96db26).
1015             pwallet->LoadAddressPreviouslySpent(dest);
1016-        } else if (strKey.compare(0, 2, "rr") == 0) {
1017+        } else if (strKey.compare(0, 2, "rr") == 0) {  // NOLINT(modernize-use-starts-ends-with)

maflcko commented at 12:53 pm on November 25, 2024:

Seems easier to just fix it right away, instead of touching the line to say to fix it and then fix it in the next commit?

l0rinc commented at 12:59 pm on November 25, 2024:

I’m fine either way, but I guess this way it was easier to selectively revert a conflicting fix and leave only the suppression.

But I’d like to understand the reason for the supressions: would the lint problem break CI without them? Since this will still invalidate ACKed PRs if we merge it like this.

hebasto commented at 1:24 pm on November 25, 2024:

Seems easier to just fix it right away, instead of touching the line to say to fix it and then fix it in the next commit?

The commits have been squashed.

But I’d like to understand the reason for the supressions: would the lint problem break CI without them?

Yes, it would.

hebasto force-pushed on Nov 25, 2024

l0rinc commented at 1:27 pm on November 25, 2024: contributor

ACK f90fddfabd646c3f1d21de7bb004707a072e753e

in src/test/result_tests.cpp:71 in f90fddfabd outdated

62@@ -63,6 +63,7 @@ void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args
63 {
64     ExpectResult(result, true, str);
65     BOOST_CHECK_EQUAL(result.has_value(), true);
66+    // NOLINTNEXTLINE(bugprone-use-after-move)

maflcko commented at 1:31 pm on November 25, 2024:

it would be good to understand why this is needed. If this is a bug, it would be good to fix it.

hebasto commented at 1:34 pm on November 25, 2024:

Right. I outlined this issue for follow-ups in the PR description.

l0rinc commented at 1:37 pm on November 25, 2024:

We can likely simplify the call here, see: #31306 (review)

hebasto commented at 1:43 pm on November 25, 2024:

Fixing all new “bugprone-use-after-move” warnings contradicts with other comments. On the other hand, fixing some of them is not consistent. That’s why I chose to silence them.

NOLINT comments are easy searchable and effectively document the issues that have to be resolved.

l0rinc commented at 2:00 pm on November 25, 2024:

Unfortunately silencing will also invalidate the ACK-ed PR. But if we could instead add -bugprone-use-after-move to src/.clang-tidy this concern could be addressed separately.

hebasto commented at 2:03 pm on November 25, 2024:

It will expose the whole codebase to re-introducing related bugs, no?

l0rinc commented at 2:08 pm on November 25, 2024:

We could tackle it in a separate pr like we do with the performance-* checks, right?

hebasto commented at 2:19 pm on November 25, 2024:

We could, but performance-* checks are not about safety, whereas bugprone-* checks are.

l0rinc commented at 2:21 pm on November 25, 2024:

If you decide to fix them here, I’ll just rebase the related PR and ask for a re-review, it’s not a tragedy :)

maflcko commented at 2:45 pm on November 25, 2024:

I looked at this and this is an upstream boost bug, not an llvm or bitcoin-core bug.

The reason is that the macro is a loop (not a do-while-0-loop):

 0#define BOOST_TEST_TOOL_IMPL( frwd_type, P, assertion_descr, TL, CT, ARGS )     \
 1do {                                                                            \
 2    BOOST_TEST_PASSPOINT();                                                     \
 3    ::boost::test_tools::tt_detail::                                            \
 4    BOOST_PP_IF( frwd_type, report_assertion, check_frwd ) (                    \
 5        BOOST_JOIN( BOOST_TEST_TOOL_PASS_PRED, frwd_type )( P, ARGS ),          \
 6        BOOST_TEST_LAZY_MSG( assertion_descr ),                                 \
 7        BOOST_TEST_L(__FILE__),                                                 \
 8        static_cast<std::size_t>(__LINE__),                                     \
 9        ::boost::test_tools::tt_detail::TL,                                     \
10        ::boost::test_tools::tt_detail::CT                                      \
11        BOOST_JOIN( BOOST_TEST_TOOL_PASS_ARGS, frwd_type )( ARGS ) );           \
12} while( ::boost::test_tools::tt_detail::dummy_cond() )                         \

maflcko commented at 2:55 pm on November 25, 2024:

The bug is fixed in https://github.com/boostorg/test/commit/eb0c07b9f004981184bbd24c435726d735eb5581

theuni commented at 6:52 pm on November 25, 2024:

NOLINT comments are easy searchable and effectively document the issues that have to be resolved.

Sorry, but no, that is exactly the opposite of what NOLINT documents!

NOLINT means “I’m smarter than my static analyzer”.

TODO would effectively document an issue to be resolved.

Hard NACK here due to the reasoning being applied. In fact, it rises to the level of suspicion. I’ll be reviewing all of these refactors in great detail.

l0rinc commented at 7:07 pm on November 25, 2024:

@hebasto, what if we invert this and push the fixes in separate PRs and only merge this TIDY_LLVM_V upgrade after all problems were already resolved?

hebasto commented at 8:00 pm on November 25, 2024:

NOLINT comments are easy searchable and effectively document the issues that have to be resolved.

Sorry, but no, that is exactly the opposite of what NOLINT documents!

NOLINT means “I’m smarter than my static analyzer”.

TODO would effectively document an issue to be resolved.

Hard NACK here due to the reasoning being applied. In fact, it rises to the level of suspicion. I’ll be reviewing all of these refactors in great detail.

A bare NOLINT, without any explanation, has never meant “I’m smarter than my static analyzer”—at least, not to me.

theuni commented at 8:11 pm on November 25, 2024:

A bare NOLINT, without any explanation, has never meant “I’m smarter than my static analyzer”—at least, not to me.

I’m not sure what else it could mean.

From the docs: “clang-tidy diagnostics are intended to call out code that does not adhere to a coding standard, or is otherwise problematic in some way. However, if the code is known to be correct, it may be useful to silence the warning”

Does this mean there are other places in the code where NOLINT is used as a means of documenting a todo? If so, we need to address those asap.

hebasto commented at 8:14 pm on November 25, 2024:

I’m not sure what else it could mean.

From the docs: “clang-tidy diagnostics are intended to call out code that does not adhere to a coding standard, or is otherwise problematic in some way. However, if the code is known to be correct, it may be useful to silence the warning”

You are right.

theuni commented at 8:33 pm on November 25, 2024:

@hebasto No need to close this if we can change the approach. I think the changes are useful (once the actual bugs are fixed), but your suggestion that other real bugs might have been previously masked set off my alarm bells. Looking through the code now (bench/tests excluded), I don’t see any other examples of this.

I think we can agree on an approach where we (in this order):

Fix the actual bugs
Notate the false-positives
Enable the checks.

It sounds like it also is worth adding a line in developer-notes.md that describes when a NOLINT is/is not appropriate.

maflcko commented at 8:29 am on November 27, 2024:

My preference would be to drop this comment blob. Otherwise, there will be bike-shedding when it is fine to remove it. Also, the code has to be touched again at that time. It seems better to just touch it once and then be done with it.

Suggested diff:

 0diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp
 1index 6a23a7b..5e18d23 100644
 2--- a/src/test/result_tests.cpp
 3+++ b/src/test/result_tests.cpp
 4@@ -63,7 +63,8 @@ void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args
 5 {
 6     ExpectResult(result, true, str);
 7     BOOST_CHECK_EQUAL(result.has_value(), true);
 8-    BOOST_CHECK_EQUAL(result.value(), T{std::forward<Args>(args)...});
 9+T expected{std::forward<Args>(args)...};
10+    BOOST_CHECK_EQUAL(result.value(), expected);
11     BOOST_CHECK_EQUAL(&result.value(), &*result);
12 }
13

(and then possibly mention the comment in the commit message)

maflcko commented at 1:47 pm on December 5, 2024:

Have you seen this suggestion?

hebasto commented at 2:39 pm on December 5, 2024:

Have you seen this suggestion?

Thanks! Your suggestion has been accepted.

hebasto closed this on Nov 25, 2024

in src/test/coins_tests.cpp:777 in 74e7813987 outdated

772@@ -773,8 +773,10 @@ static void CheckAddCoinBase(CAmount base_value, CAmount cache_value, CAmount mo
773 template <typename... Args>
774 static void CheckAddCoin(Args&&... args)
775 {
776-    for (const CAmount base_value : {ABSENT, SPENT, VALUE1})
777+    for (const CAmount base_value : {ABSENT, SPENT, VALUE1}) {
778+        // NOLINTNEXTLINE(bugprone-use-after-move)

theuni commented at 8:00 pm on November 25, 2024:

As mentioned here, this change struck me as dangerous.

It shouldn’t be marked nolint, rather it should be fixed: drop the forward and pass by const Args&.

I understand the desire to turn on new checks and notate the false-positives, but the result should always be better/safer/cleaner code.

In this case, the result would’ve been a latent bug explicitly marked notabug. And while a buggy test isn’t the end of the world, the suggestion that the approach for recent similar PR’s might have been “quiet the static analysis tool first, fix the bug later” struck me as so backwards that I had to wonder if any other actual bugs in more important code had been masked as well. Looking through recent/current PRs, I don’t see any other examples of that (modulo the boost issue pointed out here).

Please prioritize correctness and consider what you’re signaling to reviewers.

hebasto commented at 8:03 pm on November 25, 2024:

Right.

hebasto commented at 8:08 pm on November 25, 2024:

As mentioned here, this change struck me as dangerous.

My apologies.

An additional context’s available here: #31306 (review).

hebasto reopened this on Nov 26, 2024

hebasto force-pushed on Nov 26, 2024

hebasto commented at 11:04 pm on November 26, 2024: member

No need to close this if we can change the approach.

Rebased on top of the merged #31305 and #31364.

I think we can agree on an approach where we (in this order):

Fix the actual bugs

Notate the false-positives

Enable the checks.

Done.

It sounds like it also is worth adding a line in developer-notes.md that describes when a NOLINT is/is not appropriate.

Left this change for a follow-up.

in src/test/result_tests.cpp:66 in 44f8c9c876 outdated

62@@ -63,6 +63,12 @@ void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args
63 {
64     ExpectResult(result, true, str);
65     BOOST_CHECK_EQUAL(result.has_value(), true);
66+    // Boost.Test versions < 1.87 contain a bug that triggers

l0rinc commented at 3:58 pm on November 27, 2024:

I’m not sure I understand the reason for the suppression, please see my original suggestion in #31306 (review)

hebasto commented at 12:59 pm on December 3, 2024:

I’m not sure I understand the reason for the suppression

The reason of the suppression is the correctness of our code.

please see my original suggestion in #31306 (comment)

#31306 (review)

DrahtBot added the label Needs rebase on Dec 5, 2024

hebasto force-pushed on Dec 5, 2024

DrahtBot removed the label Needs rebase on Dec 5, 2024

hebasto commented at 11:27 am on December 5, 2024: member

Rebased on top of the merged #30906.

maflcko approved

maflcko commented at 1:48 pm on December 5, 2024: member

lgtm, but it would be better to apply my suggestion. Otherwise the test code will have to be touched again at some point in the future.

ci: Update Clang in "tidy" job

This change switches to the latest IWYU 0.23, which is compatible with
Clang 19.

Fixed new "modernize-use-starts-ends-with" warnings.

The new "bugprone-use-after-move" warning in `result_tests.cpp` is a
false positive caused by a bug in Boost.Test versions < 1.87. This has
been addressed by introducing a local variable.
See upstream references:
 - Issue: https://github.com/boostorg/test/issues/343
 - Fix: https://github.com/boostorg/test/pull/348

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

fe9bc5abef

iwyu: Drop backported mapping

See: https://github.com/include-what-you-use/include-what-you-use/pull/1560

31e59d94c6

hebasto force-pushed on Dec 5, 2024

hebasto commented at 2:39 pm on December 5, 2024: member

lgtm, but it would be better to apply my suggestion. Otherwise the test code will have to be touched again at some point in the future.

The suggestion has been applied.

maflcko commented at 3:23 pm on December 5, 2024: member

lgtm ACK 31e59d94c67b58f24dd701fc7ccfee7d79f311cb

DrahtBot requested review from l0rinc on Dec 5, 2024

l0rinc commented at 12:00 pm on December 6, 2024: contributor

ACK 31e59d94c67b58f24dd701fc7ccfee7d79f311cb

theuni approved

theuni commented at 6:05 pm on December 6, 2024: member

ACK 31e59d94c67b58f24dd701fc7ccfee7d79f311cb

fanquake merged this on Dec 8, 2024

fanquake closed this on Dec 8, 2024

hebasto deleted the branch on Dec 12, 2024

ci: Update Clang in “tidy” job #31306

Code Coverage & Benchmarks

Reviews

Conflicts