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.
| Type | Reviewers |
|---|---|
| Concept ACK | l0rinc |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
-Wunused by fanquake)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
rebased on top of the recent iwyu changes to drop hunks already merged, also taken out of draft
Concept ACK, though the scripted diffs will conflict with most open PRs.
I expect this to be in review for a while, so maybe we could extract the lint commit into a separate PR - that one is conflict-free and should pass quickly. And once it's in, both scripted diffs here can be done in a single commit: the formatter can just work on the pending diff right after the insertions, instead of formatting the previous commit in a separate step.
Also, since the formatter fixes placement anyway, could the include insertion skip the first-#include line-number lookup and just insert at the top of the file?
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-
Concept ACK, though the scripted diffs will conflict with most open PRs.
I went through all conflicts and there are about 6 acks in the list. Also, conflicts should be trivial to resolve in the include list, so I think it is fine. But I am happy to wait until this invalidates less acks.
I expect this to be in review for a while, so maybe we could extract the lint commit into a separate PR - that one is conflict-free and should pass quickly. And once it's in, both scripted diffs here can be done in a single commit:
The changes and scripts should be easy enough to keep bundled here. Also, I think it is easier to review the addition and the sorting separately.
could the include insertion skip the first-#include line-number lookup and just insert at the top of the file?
clang-format can't move includes over // comment lines. So this won't work.
Rebased and left as-is for now.