In test code, assert is used. This is perfectly fine, but sometimes confusion arises, when it is unclear whether NDEBUG can disable the assertions, or whether to use assert or Assert.
Avoid that confusion in test code with a scripted replacement to add the util/check.h include. The changes here will also make it easier to run IWYU.
Scope: This change is only about test code (bench, fuzz, unit), other code can be done later, if there is need to.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36074.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process. A summary of reviews will appear here.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
WalletBatch write failures by achow101)util::Expected by yuvicc)CAmount a class by hodlinator)maxfeerate wallet startup option by ismaelsadeeq)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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task i686, no IPC: https://github.com/bitcoin/bitcoin/actions/runs/32832295932/job/97753466551</sub>
<sub>LLM reason (✨ experimental): CI failed due to a C++ build error treated as fatal (-Werror=return-type) in bench/sign_transaction.cpp where an Assert(false) lambda triggers “control reaches end of non-void function.”</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
Hmm, I guess Assert(0/false) being a function trips GCC into thinking it can return? Though, the optimized codegen is unaffected on GCC. Funnily clang doesn't warn, but pessimises codegen: https://godbolt.org/z/h79K1scWK
I guess that means we could add an explicit #define AssertUnreachable() assertion_fail(std::source_location::current(), "Unreachable code")
124 | - assert(!mini_miner.IsReadyToCalculate()); 125 | + Assert(total_bumpfee.has_value()); 126 | + Assert(!mini_miner.IsReadyToCalculate()); 127 | } 128 | // Overlapping ancestry across multiple outpoints can only reduce the total bump fee. 129 | assert (sum_fees >= *total_bumpfee);
This one doesn't match \<assert\( because of the space, and it's the only one left in the swept paths. lint_c_assert uses the same regex, so it doesn't catch it either. I added assert (1); to a covered file and the linter still passes. How about \<assert\s*\(?
Thx, but I've dropped the third and forth commit for now, because:
This is why I only migrated the problematic usages with side effects - but I'm glad you're taking a stab at it systemically, even if just for tests for now
159 | + "--", 160 | + // Limit to test-only code for now: 161 | + "src/test/", 162 | + "src/bench/", 163 | + "src/wallet/test/", 164 | + "src/qt/test/",
src/ipc/test/ is left out here, and src/ipc/test/fuzz/ipc.cpp has six assert(. lint-tests.py already counts that directory as part of the test suite. Is this intentional?
The comma in 'src/ipc/test/', ends up inside the element, so it still matches nothing.
The comma in
'src/ipc/test/',ends up inside the element, so it still matches nothing.
Whoops, fixed. Thx
The project has a compile error when compiled without assertions in
util/check.h. Thus, util/check.h should be included for all assertions.
So do that with a scripted-diff for Assert and assert, and remove the
cassert include, which is exported from util/check.h.
-BEGIN VERIFY SCRIPT-
# Select all test .cpp and .h files
paths=(
'src/bench/'
'src/ipc/test/'
'src/qt/test/'
'src/test/'
'src/wallet/test/'
':(exclude)src/bench/nanobench.h'
)
# Add the util/check.h includes
for f in $(git grep -l --extended-regexp "\<(a|A)ssert\(" -- "${paths[@]}"); do
if ! grep --quiet "util/check.h" "$f"; then
line=$(grep --line-number --max-count=1 '^#include' "$f" | cut --delimiter=: --fields=1)
sed --in-place "${line}i#include <util/check.h>" "$f"
fi
done
# Remove cassert includes
for f in $(git grep -l '<cassert>' -- "${paths[@]}"); do
sed --in-place '/^#include <cassert>$/d' "$f"
done
-END VERIFY SCRIPT-
Also, remove the unused pushd/popd. No command in this file requires a
special PWD.
-BEGIN VERIFY SCRIPT-
git show -U0 HEAD~1 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
-END VERIFY SCRIPT-
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.