mirror of
https://github.com/bitcoin/bitcoin.git
synced 2026-05-29 15:23:26 +02:00
Merge bitcoin/bitcoin#35152: doc: clarify local IWYU workflow and pragmas
d084bc88bedoc: clarify IWYU workflow (Lőrinc)7c7cec4567ci: update IWYU patch reference (Lőrinc) Pull request description: ### Problem This was prompted by https://github.com/bitcoin/bitcoin/pull/34435#discussion_r3123255248, where it was not clear to me how (and where) exceptional IWYU cases should be documented. ### Fix This PR documents the IWYU CI wrapper as the reproducible local entrypoint. The developer notes now recommend reducing suspected IWYU false positives to a minimal upstream reproducer, treat `IWYU pragma` as a narrow workarounds, and ask for nearby rationale comments on non-obvious IWYU pragma use. An example comment was also added. The IWYU patch comment is also updated to point at the current `clang_22` include picker reference. ### Reproducer Create a dummy commit on top that adds an unused include, then run the command from the developer notes. Without the dummy commit, the command should pass. <details><summary>IWYU demo commit</summary> ```diff diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp --- a/src/kernel/bitcoinkernel.cpp(revision c92b329e7b7d49476b5977d26c24d7c4982c6024) +++ b/src/kernel/bitcoinkernel.cpp(revision ad2c5ba2ba69156e77061c1e6c098b725c28f322) @@ -43,6 +43,7 @@ #include <functional> #include <list> #include <memory> +#include <vector> #include <span> #include <stdexcept> #include <string> ``` </details> > [!NOTE] > After repeated failing runs, `docker container rm -f ci_native_iwyu` may be needed because the local CI wrapper can leave the detached container running when the inner test command fails. ACKs for top commit: hebasto: ACKd084bc88be. sedited: ACKd084bc88beTree-SHA512: 0aac42d468a1fdfa9f4a3856372e05fb43ec9f0973aeb3a4194fff948fc61e8e72e3b280cde10e74b8da88b6cff93962b3a7f7390eb042113ef92aa6b51d6d8f
This commit is contained in:
@@ -15,7 +15,7 @@ See: https://en.cppreference.com/w/cpp/preprocessor/include.html.
|
||||
|
||||
|
||||
Prefer C++ headers over C counterparts.
|
||||
See: https://github.com/include-what-you-use/include-what-you-use/blob/clang_21/iwyu_include_picker.cc#L587-L629.
|
||||
See: https://github.com/include-what-you-use/include-what-you-use/blob/clang_22/iwyu_include_picker.cc#L645-L687.
|
||||
|
||||
--- a/iwyu_include_picker.cc
|
||||
+++ b/iwyu_include_picker.cc
|
||||
|
||||
@@ -539,22 +539,32 @@ The generated coverage report can be accessed at `build/coverage_report/index.ht
|
||||
The [`include-what-you-use`](https://github.com/include-what-you-use/include-what-you-use) tool (IWYU)
|
||||
helps to enforce the source code organization [policy](#source-code-organization) in this repository.
|
||||
|
||||
To ensure consistency, it is recommended to run the IWYU CI job locally rather than running the tool directly.
|
||||
To reproduce the IWYU CI job locally, run:
|
||||
```bash
|
||||
env -i HOME="$HOME" PATH="$PATH" USER="$USER" MAKEJOBS="-j1" FILE_ENV="./ci/test/00_setup_env_native_iwyu.sh" ./ci/test_run_all.sh || echo "IWYU failed"
|
||||
```
|
||||
|
||||
In some cases, IWYU might suggest headers that seem unnecessary at first glance, but are actually required.
|
||||
For example, a macro may use a symbol that requires its own include. Another example is passing a string literal
|
||||
to a function that accepts a `std::string` parameter. An implicit conversion occurs at the callsite using the
|
||||
`std::string` constructor, which makes the corresponding header required. We accept these suggestions as is.
|
||||
|
||||
Use `IWYU pragma: export` very sparingly, as this enforces transitive inclusion of headers
|
||||
and undermines the specific purpose of IWYU.
|
||||
If the provided IWYU CI job still produces a false positive, reduce it to a minimal reproducer and report it upstream.
|
||||
|
||||
Use IWYU pragmas sparingly.
|
||||
|
||||
Use `IWYU pragma: keep` only as a narrow workaround when needed.
|
||||
|
||||
Use `IWYU pragma: associated` only when IWYU cannot infer the intended associated header.
|
||||
|
||||
Use `IWYU pragma: export` very sparingly, as this enforces transitive inclusion of headers and undermines the specific purpose of IWYU.
|
||||
|
||||
The acceptable cases for using `IWYU pragma: export` are:
|
||||
1. Facade headers. For example, see [`compat/compat.h`](/src/compat/compat.h).
|
||||
2. Drop-in replacement headers. For example, see [`util/time.h`](/src/util/time.h).
|
||||
3. Presenting a complete interface across multiple headers.
|
||||
|
||||
A comment explaining the rationale is required for every use of `IWYU pragma: export`.
|
||||
For IWYU pragmas, prefer adding a nearby source comment that explains why the annotation is needed.
|
||||
|
||||
### Performance profiling with perf
|
||||
|
||||
|
||||
@@ -18,6 +18,8 @@
|
||||
#include <streams.h>
|
||||
#include <sync.h>
|
||||
#include <uint256.h>
|
||||
// IWYU incorrectly suggests removing this header.
|
||||
// See https://github.com/include-what-you-use/include-what-you-use/issues/2014.
|
||||
#include <util/byte_units.h> // IWYU pragma: keep
|
||||
#include <util/expected.h>
|
||||
#include <util/fs.h>
|
||||
|
||||
Reference in New Issue
Block a user