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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31306.
See the guideline for information on the review process.
Type | Reviewers |
---|---|
Stale ACK | l0rinc |
If your review is incorrectly listed, please react with π to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
18@@ -19,8 +19,10 @@ performance-*,
19 -performance-avoid-endl,
20 -performance-enum-size,
21 -performance-inefficient-string-concatenation,
22+-performance-inefficient-vector-operation,
Right.
It depends on which PR is merged first :)
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"
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
APT_LLVM_V
.
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...});
ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S"));
?
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)
c4eb31b
(#30906) removes this method completely, which I think is a better fix for it than the current workaround.
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)
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}
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.
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,
ACK 3e039b7a03bea6e11e994aebe038ebd6ca80aeb8
The latest change:
CI_IMAGE_NAME_TAG
and added APT_LLVM_V
.CheckAddCoin
and ExpectSuccess
fixes to avoid collisions with other PRs.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)
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.
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.
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)
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.
-bugprone-use-after-move
to src/.clang-tidy
this concern could be addressed separately.
performance-*
checks, right?
performance-*
checks are not about safety, whereas bugprone-*
checks are.
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() ) \
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.
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.
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.
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.
@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):
It sounds like it also is worth adding a line in developer-notes.md
that describes when a NOLINT
is/is not appropriate.
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)
This change 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.
See: https://github.com/include-what-you-use/include-what-you-use/pull/1560
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)
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.
As mentioned here, this change struck me as dangerous.
My apologies.
An additional context’s available here: #31306 (review).
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 aNOLINT
is/is not appropriate.
Left this change for a follow-up.