Fixes #34237
Currently, there is a massive iwyu patch to try to implement modernize-deprecated-headers.
However, it does not work. For example https://github.com/bitcoin/bitcoin/actions/runs/21012214401/job/60409645985#step:11:35548:
0
1+#include <assert.h>
2 #include <bench/bench.h>
Fix all issues by removing the patch and using a trivial and fast modernize-deprecated-headers.py instead.
This reverts commit 73f7844cdb1e225099223a355d88da0522d7d69b.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34298.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hebasto |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
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.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21025279080/job/60448207726
LLM reason (✨ experimental): Lint check failed in CI due to shellcheck SC2046 warning causing the lint suite to fail.
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.
The CI logs show that the example from the pull description is now working correctly: https://github.com/bitcoin/bitcoin/actions/runs/21027399192/job/60455097235?pr=34298#step:11:38466:
0--- a/src/bench/streams_findbyte.cpp
1+++ b/src/bench/streams_findbyte.cpp
2@@ -7,9 +7,12 @@
3 #include <test/util/setup_common.h>
4 #include <util/fs.h>
5
6+#include <cassert>
7 #include <cstddef>
The CI logs show that the example from the pull description is now working correctly: https://github.com/bitcoin/bitcoin/actions/runs/21027399192/job/60455097235?pr=34298#step:11:38466:
0--- a/src/bench/streams_findbyte.cpp 1+++ b/src/bench/streams_findbyte.cpp 2@@ -7,9 +7,12 @@ 3 #include <test/util/setup_common.h> 4 #include <util/fs.h> 5 6+#include <cassert> 7 #include <cstddef>
However, IWYU output still mentions C headers:
02026-01-15T10:19:53.0393770Z /home/admin/actions-runner/_work/_temp/src/bench/streams_findbyte.cpp should add these lines:
12026-01-15T10:19:53.0394044Z #include <assert.h> // for assert
22026-01-15T10:19:53.0394243Z #include <memory> // for make_unique, unique_ptr
32026-01-15T10:19:53.0394460Z #include <string> // for basic_string
Currently, there is a massive iwyu patch to try to implement
modernize-deprecated-headers.However, it does not work.
The patch has been fixed in #34310.
However, IWYU output still mentions C headers:
Yeah, this will also be the case with a vanilla iwyu, when run locally. Not sure if anyone does this, but the py script could be applicable there as well. Not sure if anyone runs iwyu locally, and if they’d prefer a patch to iwyu, or a fixup script, but happy to close this one, if people prefer a patch.
I am happy with both approaches, this one and #34310, though I have a slight preference for the latter for the following reasons:
If you did want to replace assert.h with cassert, you’d change it to a public->private mapping.
If the patch size is an issue, I’d prefer the previous “scripted-diff” style script to modify the source code.
Labels
Tests
Needs rebase