ci, iwyu: Treat warnings as errors for specific directories

hebasto commented at 9:00 pm on November 17, 2024: member

This PR is the first step towards treating IWYU warnings as errors. At this stage, it applies only to the crypto and index directories.

hebasto added the label Refactoring on Nov 17, 2024

hebasto added the label Tests on Nov 17, 2024

DrahtBot commented at 9:00 pm on November 17, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31308.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko
Concept ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32694 (index: move disk read lookups to base class by furszy)
#32673 (clang-tidy: Apply modernize-deprecated-headers by maflcko)
#32189 (refactor: Txid type safety (parent PR) by marcofleon)
#29307 (util: explicitly close all AutoFiles that have been written by vasild)
#26966 (index: initial sync speedup, parallelize process by furszy)

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.

hebasto force-pushed on Nov 18, 2024

DrahtBot added the label CI failed on Nov 18, 2024

hebasto force-pushed on Nov 25, 2024

DrahtBot added the label Needs rebase on Dec 5, 2024

hebasto force-pushed on Dec 5, 2024

DrahtBot removed the label Needs rebase on Dec 5, 2024

hebasto marked this as ready for review on Dec 5, 2024

DrahtBot removed the label CI failed on Dec 5, 2024

DrahtBot added the label Needs rebase on Dec 8, 2024

hebasto force-pushed on Dec 12, 2024

hebasto commented at 10:45 am on December 12, 2024: member

Rebased.

DrahtBot removed the label Needs rebase on Dec 12, 2024

in ci/test/03_test_script.sh:186 in d838f1bac6 outdated

180@@ -180,6 +181,13 @@ if [ "${RUN_TIDY}" = "true" ]; then
181            -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
182            -Xiwyu --max_line_length=160 \
183            2>&1 | tee /tmp/iwyu_ci.out
184+
185+  # TODO: Expand the following list to all targets.
186+  TARGETS_WITH_FORCED_IWYU="bitcoin_clientversion bitcoin_crypto"
187+  cd "${BASE_BUILD_DIR}"
188+  bash -c "cmake -S ${BASE_ROOT_DIR} -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE=\"include-what-you-use;-Xiwyu;--cxx17ns;-Xiwyu;--mapping_file=${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp;-Xiwyu;--error\""
189+  bash -c "cmake --build . ${MAKEJOBS} --clean-first --target ${TARGETS_WITH_FORCED_IWYU}"

maflcko commented at 10:10 pm on December 19, 2024:

Could make sense to run this before the tee /tmp/iwyu_ci.out command, given that it errors? Or would it be missing (part of) the output? It would be good to see an example CI run. Maybe a -- --keep-going could be used for a slightly better output, if stuff is possibly missing?

hebasto commented at 10:28 am on January 20, 2025:

Could make sense to run this before the tee /tmp/iwyu_ci.out command, given that it errors?

This approach allows a developer to check other includes, not only the first one that fails.

Maybe a -- --keep-going could be used for a slightly better output, if stuff is possibly missing?

I’m not sure what criteria you are using to define “better” in this case. The current branch adds one more config/build round details to the CI log and the following:

A message like the one posted in the PR description, if there is a warning.
Nothing, if there are no warnings.

maflcko commented at 11:00 am on January 20, 2025:

I think the message you posted in the PR description only shows a single error in a single dependency. I am wondering what happens if there is more than one error in different targets, possibly ones where cmake will never get to print the second error.

I am asking, because after the cmake migration the behavior of make -k changed. Previously, everything would be attempted to be built. Now, cmake will skip and exit early in some cases.

hebasto commented at 4:37 pm on May 22, 2025:

I think the message you posted in the PR description only shows a single error in a single dependency. I am wondering what happens if there is more than one error in different targets, possibly ones where cmake will never get to print the second error.

Here multiple error are reported.

maflcko commented at 2:58 pm on May 26, 2025:

I can only see 5 reported errors, however, there should be 8 in total, no?

hebasto commented at 2:09 pm on May 27, 2025:

The change touches 8 files. The modification in src/crypto/siphash.h is a no-op from IWYU’s perspective.
CMake integrates the IWYU into the compilation step. Therefore, it reports an error for each object file individually. However, the diagnostic output includes both headers and the source file:

 0[12:32:50.757] Warning: include-what-you-use reported diagnostics:
 1[12:32:50.757] 
 2[12:32:50.757] /ci_container_base/src/crypto/poly1305.h should add these lines:
 3[12:32:50.757] #include <span>      // for span
 4[12:32:50.757] 
 5[12:32:50.757] /ci_container_base/src/crypto/poly1305.h should remove these lines:
 6[12:32:50.757] 
 7[12:32:50.757] The full include-list for /ci_container_base/src/crypto/poly1305.h:
 8[12:32:50.757] #include <span.h>    // for UCharCast
 9[12:32:50.757] #include <stdint.h>  // for uint32_t
10[12:32:50.757] #include <cassert>   // for assert
11[12:32:50.757] #include <cstddef>   // for byte, size_t
12[12:32:50.757] #include <span>      // for span
13[12:32:50.757] ---
14[12:32:50.757] 
15[12:32:50.757] (/ci_container_base/src/crypto/poly1305.cpp has correct #includes/fwd-decls)
16[12:32:50.757] 
17[12:32:50.757] gmake[3]: *** [src/crypto/CMakeFiles/bitcoin_crypto.dir/build.make:188: src/crypto/CMakeFiles/bitcoin_crypto.dir/poly1305.cpp.o] Error 1

I can see all changes, except the unrelated one, in the IWYU diagnostics. Am I missing anything in particular?

maflcko commented at 12:08 pm on May 28, 2025:

Ok, fair enough. It may be fine, and should be trivial to revert, if it doesn’t work as expected.

However, I worry a bit about the output not being copy-paste-able for developers to apply to their source code.

Not sure if this is a valid concern, but an alternative approach would be to just call git diff below on the TARGETS_WITH_FORCED_IWYU folders (not targets). Then exit, when the diff is not empty?

This should also address the issue of having to run twice.

The only downside I see is that the iwyu debug comments (// for UCharCast) have to be looked up by scrolling up, but this should be fine.

hebasto commented at 11:16 am on May 29, 2025:

Could we defer such amendments to follow-ups until feedback from real-world CI usage has been collected?

hebasto commented at 5:12 pm on May 29, 2025:

Not sure if this is a valid concern, but an alternative approach would be to just call git diff below on the TARGETS_WITH_FORCED_IWYU folders (not targets). Then exit, when the diff is not empty?

Thanks! Reworked.

DrahtBot added the label Needs rebase on Jan 27, 2025

hebasto marked this as a draft on Feb 21, 2025

maflcko commented at 7:08 am on May 21, 2025: member

concept ack, but yeah, needs rebase

hebasto force-pushed on May 22, 2025

DrahtBot removed the label Needs rebase on May 22, 2025

hebasto force-pushed on May 22, 2025

DrahtBot added the label CI failed on May 22, 2025

DrahtBot commented at 4:35 pm on May 22, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/42720827740 LLM reason (✨ experimental): The CI failure is due to build errors related to include-what-you-use diagnostics.

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.

hebasto marked this as ready for review on May 22, 2025

hebasto commented at 4:49 pm on May 22, 2025: member

Rebased.

DrahtBot removed the label CI failed on May 22, 2025

in ci/test/03_test_script.sh:185 in 0c3c7d0759 outdated

177@@ -178,6 +178,13 @@ if [ "${RUN_TIDY}" = "true" ]; then
178            -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
179            -Xiwyu --max_line_length=160 \
180            2>&1 | tee /tmp/iwyu_ci.out
181+
182+  # TODO: Expand the following list to all targets.
183+  TARGETS_WITH_FORCED_IWYU="bitcoin_clientversion bitcoin_crypto"
184+  cd "${BASE_BUILD_DIR}"
185+  bash -c "cmake -S ${BASE_ROOT_DIR} -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE=\"include-what-you-use;-Xiwyu;--cxx17ns;-Xiwyu;--mapping_file=${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp;-Xiwyu;--error\""

maflcko commented at 12:03 pm on May 28, 2025:

nit: no need to disable shellcheck here via bash -c?

hebasto commented at 2:14 pm on May 28, 2025:

Thanks! Done.

in ci/test/03_test_script.sh:186 in 0c3c7d0759 outdated

177@@ -178,6 +178,13 @@ if [ "${RUN_TIDY}" = "true" ]; then
178            -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
179            -Xiwyu --max_line_length=160 \
180            2>&1 | tee /tmp/iwyu_ci.out
181+
182+  # TODO: Expand the following list to all targets.
183+  TARGETS_WITH_FORCED_IWYU="bitcoin_clientversion bitcoin_crypto"
184+  cd "${BASE_BUILD_DIR}"
185+  bash -c "cmake -S ${BASE_ROOT_DIR} -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE=\"include-what-you-use;-Xiwyu;--cxx17ns;-Xiwyu;--mapping_file=${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp;-Xiwyu;--error\""
186+  bash -c "cmake --build . ${MAKEJOBS} --clean-first --target ${TARGETS_WITH_FORCED_IWYU}"

maflcko commented at 12:04 pm on May 28, 2025:

nit: Could inline the TARGETS_WITH_FORCED_IWYU to avoid the bash -c disable of shellcheck?

hebasto commented at 2:14 pm on May 28, 2025:

Thanks! Done.

maflcko commented at 12:09 pm on May 28, 2025: member

left some comments

hebasto force-pushed on May 28, 2025

fanquake commented at 11:28 am on May 29, 2025: member

While this approach introduces some duplication at present, it will ultimately enable us to consolidate how the IWYU tool is executed, allowing for a choice between using the iwyu_tool.py script or CMake’s built-in functionality.

Reading the PR description, it’s not really clear why we’d need to retain using both tools? Either the CMake built-in should be better in some way, or at least equivalent in functionality, in which case we should just switch to using it?

in contrib/devtools/iwyu/bitcoin.core.imp:10 in f70ffa4a9c outdated

 6+  { include: [ "<tmmintrin.h>", private, "<immintrin.h>", public ] },
 7+
 8+  # False positive warnings
 9+  # See: https://github.com/include-what-you-use/include-what-you-use/issues/1616
10+  { include: [ "<iterator>", public, "<utility>", public ] },
11+  { include: [ "<tuple>", public, "<array>", public ] },

fanquake commented at 11:29 am on May 29, 2025:

Is there an issue open for this upstream?

fanquake commented at 4:42 pm on June 2, 2025:

?

hebasto commented at 2:01 pm on June 3, 2025:

Is there an issue open for this upstream?

Please see https://github.com/include-what-you-use/include-what-you-use/issues/1763.

hebasto commented at 2:02 pm on June 3, 2025:

The issue could be worked around in different ways. For example, by using an OS image with the default GCC >= 14.

hebasto commented at 4:56 pm on June 3, 2025:

Reworked into a local pragma.

in contrib/devtools/iwyu/bitcoin.core.imp:4 in f70ffa4a9c outdated

0@@ -1,3 +1,11 @@
1-# Nothing for now.
2 [
3+  # Compiler intrinsics
4+  { include: [ "<emmintrin.h>", private, "<immintrin.h>", public ] },

fanquake commented at 11:29 am on May 29, 2025:

Same here?

fanquake commented at 4:43 pm on June 2, 2025:

?

hebasto commented at 6:16 pm on June 2, 2025:

As they form part of Intel’s API, these headers are not covered by IWYU’s mappings for the standard libraries, and in my view, they do not fall within the scope of the IWYU repository.

hebasto commented at 6:16 pm on June 2, 2025:

As they form part of Intel’s API, these headers are not covered by IWYU’s mappings for the standard libraries, and in my view, they do not fall within the scope of the IWYU repository.

fanquake commented at 6:20 pm on June 2, 2025:

they do not fall within the scope of the IWYU repository.

IWYU has mappings for intrinsics though?

hebasto commented at 3:09 pm on June 3, 2025:

Same here?

Please see https://github.com/include-what-you-use/include-what-you-use/issues/1764.

hebasto force-pushed on May 29, 2025

hebasto renamed this:
~~ci, iwyu: Treat warnings as errors for specific targets~~
ci, iwyu: Treat warnings as errors for specific directories
on May 29, 2025

hebasto commented at 5:11 pm on May 29, 2025: member

While this approach introduces some duplication at present, it will ultimately enable us to consolidate how the IWYU tool is executed, allowing for a choice between using the iwyu_tool.py script or CMake’s built-in functionality.

Reading the PR description, it’s not really clear why we’d need to retain using both tools? Either the CMake built-in should be better in some way, or at least equivalent in functionality, in which case we should just switch to using it?

I believe that the current approach, using a compilation database, is more suitable for this project than CMake’s co-compilation. The only reason for employing the latter in this PR was to use it as a means of gradually fixing includes on a per-target basis.

I’ve reworked this PR to drop CMake’s co-compilation approach in favour of @maflcko’s idea.

in ci/test/03_test_script.sh:182 in bd9e37d2ab outdated

177@@ -178,9 +178,8 @@ if [ "${RUN_TIDY}" = "true" ]; then
178            -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
179            -Xiwyu --max_line_length=160 \
180            2>&1 | tee /tmp/iwyu_ci.out
181-  cd "${BASE_ROOT_DIR}/src"
182   python3 "/include-what-you-use/fix_includes.py" --nosafe_headers < /tmp/iwyu_ci.out
183-  git --no-pager diff
184+  git --no-pager diff -- ':(exclude)src/leveldb/db/db_impl.cc'

maflcko commented at 2:47 pm on June 2, 2025:

nit in the first commit: Seems odd to exclude this without explanation in the code. Would an alternative be:

 0diff --git a/ci/test/03_test_script.sh b/ci/test/03_test_script.sh
 1index 3394c50b8b..db3d3d64de 100755
 2--- a/ci/test/03_test_script.sh
 3+++ b/ci/test/03_test_script.sh
 4@@ -33,7 +33,9 @@ echo "=== BEGIN env ==="
 5 env
 6 echo "=== END env ==="
 7 
 8-(
 9+if [ "$RUN_TIDY" != "true" ]; then
10+  # The tidy task has no UB detector, so skip the patch there to avoid issues
11+  # later on in the task.
12   # compact->outputs[i].file_size is uninitialized memory, so reading it is UB.
13   # The statistic bytes_written is only used for logging, which is disabled in
14   # CI, so as a temporary minimal fix to work around UB and CI failures, leave
15@@ -54,7 +56,7 @@ echo "=== END env ==="
16    mutex_.Lock();
17    stats_[compact->compaction->level() + 1].Add(stats);
18 EOF
19-)
20+fi
21 
22 if [ "$RUN_FUZZ_TESTS" = "true" ]; then
23   export DIR_FUZZ_IN=${DIR_QA_ASSETS}/fuzz_corpora/

?

hebasto commented at 5:03 pm on June 2, 2025:

Thanks! Reworked.

in ci/test/03_test_script.sh:181 in bd9e37d2ab outdated

177@@ -178,9 +178,8 @@ if [ "${RUN_TIDY}" = "true" ]; then
178            -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
179            -Xiwyu --max_line_length=160 \
180            2>&1 | tee /tmp/iwyu_ci.out
181-  cd "${BASE_ROOT_DIR}/src"

maflcko commented at 2:47 pm on June 2, 2025:

q in the first commit: This is no longer needed, because cmake uses absolute paths in the compile commands?

hebasto commented at 4:08 pm on June 3, 2025:

I’ve tested the pre-CMake master branch @ 80f00cafdeef0600fa1a5e906686720786c2336c, and this line wasn’t necessary there.

in src/index/coinstatsindex.h:11 in 5cd4bbb67c outdated

 4@@ -5,11 +5,20 @@
 5 #ifndef BITCOIN_INDEX_COINSTATSINDEX_H
 6 #define BITCOIN_INDEX_COINSTATSINDEX_H
 7 
 8+#include <consensus/amount.h>
 9 #include <crypto/muhash.h>
10 #include <index/base.h>
11+#include <primitives/block.h>

maflcko commented at 3:17 pm on June 2, 2025:

5cd4bbb67c3c7e651b3d83c77ffb936f60a2c496: Looks wrong. This should be a fwd dcl

hebasto commented at 5:03 pm on June 2, 2025:

Sure! Fixed.

in src/crypto/hmac_sha256.cpp:8 in eb4be21cc0 outdated

3@@ -4,6 +4,7 @@
4 
5 #include <crypto/hmac_sha256.h>
6 
7+#include <crypto/sha256.h>
8 #include <string.h>

maflcko commented at 3:22 pm on June 2, 2025:

nit in eb4be21cc0bb93b0931a61b2af342946f09a9721: Wrong section and should be cstring to avoid confusing with string.h in our repo?

hebasto commented at 5:04 pm on June 2, 2025:

Thanks! Fixed.

in src/crypto/hmac_sha512.cpp:8 in eb4be21cc0 outdated

3@@ -4,6 +4,7 @@
4 
5 #include <crypto/hmac_sha512.h>
6 
7+#include <crypto/sha512.h>
8 #include <string.h>

maflcko commented at 3:22 pm on June 2, 2025:

same…

hebasto commented at 5:04 pm on June 2, 2025:

Thanks! Fixed.

in ci/test/03_test_script.sh:193 in e9143a68f3 outdated

194+             2>&1 | tee /tmp/iwyu_ci.out
195+    python3 "/include-what-you-use/fix_includes.py" --nosafe_headers < /tmp/iwyu_ci.out
196+  }
197+
198+  run_iwyu "compile_commands_iwyu_errors.json"
199+  git diff --quiet -- ':(exclude)src/leveldb/db/db_impl.cc' || { git --no-pager diff -- ':(exclude)src/leveldb/db/db_impl.cc'; exit 1; }

maflcko commented at 3:28 pm on June 2, 2025:

e9143a68f34f350d7fa4405389222da4dfa9a394: Why not just git diff --no-pager --exit-code?

hebasto commented at 5:04 pm on June 2, 2025:

Thanks! Reworked.

maflcko commented at 3:28 pm on June 2, 2025: member

review ACK e9143a68f34f350d7fa4405389222da4dfa9a394 🎰

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK e9143a68f34f350d7fa4405389222da4dfa9a394 🎰
3fi/hWQQqCAdnfz0F4yhi8AMYjYOl79DanoxZsC6Am2gCztYYNf/izs+yQjc7oCt14KzsBJkAcSvQN6BVRVniAQ==

hebasto force-pushed on Jun 2, 2025

in ci/test/03_test_script.sh:37 in ba1d76d7bb outdated

32@@ -33,7 +33,9 @@ echo "=== BEGIN env ==="
33 env
34 echo "=== END env ==="
35 
36-(
37+if [ "$RUN_TIDY" != "true" ]; then
38+  # The tidy task has no UB detector, so skip the patch there to avoid issues

fanquake commented at 4:40 pm on June 2, 2025:

to avoid issues

What issues? Do I understand correctly, that if we land the patch in the subtree, this will just break again?

hebasto commented at 4:41 pm on June 2, 2025:

The following git diff prints this patch even IWYU did not apply any modifications.

fanquake commented at 5:14 pm on June 2, 2025:

Ok, so no an issue if we actually land the patch (and remove the related code in the CI).

maflcko commented at 6:33 am on June 3, 2025:

Doesn’t matter much, but the comment could also say “… so skip the patch because it is not needed and to avoid issues …”.

hebasto commented at 4:55 pm on June 3, 2025:

Doesn’t matter much, but the comment could also say “… so skip the patch because it is not needed and to avoid issues …”.

Thanks! Adjusted.

hebasto force-pushed on Jun 2, 2025

hebasto marked this as a draft on Jun 2, 2025

hebasto marked this as ready for review on Jun 2, 2025

hebasto force-pushed on Jun 2, 2025

in ci/test/03_test_script.sh:177 in 92ac4c8353 outdated

173@@ -174,14 +174,27 @@ if [ "${RUN_TIDY}" = "true" ]; then
174     false
175   fi
176 
177+  # TODO: Consider enforcing IWYU across the entire codebase.

jonatack commented at 5:13 pm on June 2, 2025:

Last I looked, enforcing this across the codebase may require large changes at one time in some areas to untangle everything.

jonatack commented at 5:15 pm on June 2, 2025: member

Concept ACK

maflcko commented at 6:31 am on June 3, 2025: member

only changes are addressed nits

re-review ACK 92ac4c8353c612549afbd68a4db6dfb04cc41dad 🍞

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-review ACK 92ac4c8353c612549afbd68a4db6dfb04cc41dad 🍞
3xIdLhdsyYvMngIMaaDU1eiGvAgl1sm5eg7S5tdSf7AK3oTKfKIk7+1i0wvSWq5ufz6uOZG6iKygZDc7JHnCqCg==

DrahtBot requested review from jonatack on Jun 3, 2025

ci: Do not patch `leveldb` to workaround UB in "tidy" CI job 8542900eae

refactor: Fix includes in `crypto` directory

IWYU issue #1763 appears to be a corner case, so it has been addressed
using a local pragma rather than a global mapping.

d256508fd2

refactor: Fix includes in `index` directory 7ad6563b42

ci, iwyu: Treat warnings as errors for specific directories

Currently, this applies only to the `crypto` and `index` directories.

ec42fb489a

hebasto force-pushed on Jun 3, 2025

hebasto commented at 4:55 pm on June 3, 2025: member

Rebased.

All feedback from @maflcko and @fanquake has been addressed.

The IWYU-related workarounds are now documented with references to the relevant upstream issues.

in contrib/devtools/iwyu/bitcoin.core.imp:10 in ec42fb489a

 6+  { include: [ "<smmintrin.h>", private, "<immintrin.h>", public ] },
 7+  { include: [ "<tmmintrin.h>", private, "<immintrin.h>", public ] },
 8+
 9+  # False positive warnings
10+  # See: https://github.com/include-what-you-use/include-what-you-use/issues/1616
11+  { include: [ "<iterator>", public, "<utility>", public ] },

maflcko commented at 6:47 am on June 4, 2025:

I don’t think this is right. This can lead to missing includes:

 0bash-5.2# cat map.imp 
 1[
 2  { include: [ "<iterator>", public, "<utility>", public ] },
 3]
 4bash-5.2# cat repr.cpp 
 5#include <utility>
 6#include <iterator>
 7
 8void A(){
 9	int a{};
10	int b{std::move(a)};
11	auto* it{&b};
12	std::advance(it,0);
13}
14bash-5.2# include-what-you-use  -Xiwyu --mapping_file=map.imp repr.cpp 
15
16repr.cpp should add these lines:
17
18repr.cpp should remove these lines:
19- #include <iterator>  // lines 2-2
20
21The full include-list for repr.cpp:
22#include <utility>  // for move, advance
23---
24bash-5.2# vim repr.cpp # remove iterator include
25bash-5.2# include-what-you-use  -Xiwyu --mapping_file=map.imp repr.cpp 
26repr.cpp:8:7: error: no member named 'advance' in namespace 'std'
27    8 |         std::advance(it,0);
28      |         ~~~~~^

The correct fix is to just add the includes iwyu is asking for

in src/crypto/hex_base.cpp:12 in ec42fb489a

 4@@ -5,8 +5,11 @@
 5 #include <crypto/hex_base.h>
 6 
 7 #include <array>
 8+#include <cassert>
 9 #include <cstring>
10 #include <string>
11+// A workaround for https://github.com/include-what-you-use/include-what-you-use/issues/1763.
12+// IWYU pragma: no_include <tuple>

maflcko commented at 6:49 am on June 4, 2025:

I don’t think this is right. Can’t this lead to missing the tuple include when it is required in the future?

The correct fix is to just add the includes iwyu is asking for

maflcko commented at 9:45 am on June 7, 2025: member

lgtm, except for the two missing includes

re-ACK ec42fb489a7eabed4dec155c6655d36114fa67ba 👜

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK ec42fb489a7eabed4dec155c6655d36114fa67ba 👜
3FEGC/u3sOGwaQIusYwFrEp5xYc5la53gzpRtrrSaUlHqwWAcpeIzsyPV2yxGl8A7HQezMFSpBdLyeqSk7dACAw==

achow101 referenced this in commit 5757de4ddd on Jun 11, 2025

DrahtBot added the label Needs rebase on Jun 11, 2025

DrahtBot commented at 10:28 pm on June 11, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

ci, iwyu: Treat warnings as errors for specific directories #31308

Code Coverage & Benchmarks

Reviews

Conflicts