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

hebasto · 2024-11-17T21:00:19Z

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

DrahtBot · 2024-11-17T21:00:22Z

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	ryanofsky
Concept ACK	jonatack, l0rinc, sipa
Stale ACK	maflcko

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:

#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 · 2024-12-12T10:45:41Z

Rebased.

ci/test/03_test_script.sh

maflcko · 2025-09-01T08:10:31Z

Only change is rebase.

re-ACK a4fa7b9 🚇

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK a4fa7b97aa73b177ea416388acfc2aba25c79e19 🚇
bRpP979EiaRYiW2rw/6yGXWljok64xDpRXXHzbyfOOiBMN+ftjgfNugbR0of17pwNoW+kOx/kZxEgzXhOkWhAQ==

ci/test/03_test_script.sh

hebasto · 2025-09-01T10:34:35Z

Rebased and addressed @maflcko's comment.

maflcko · 2025-09-01T11:57:23Z

re-ACK 1d49346 📶

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 1d49346465de8edec85bb2d498859b4fbc346935 📶
2MJL8GNQQjZb4FgXDGl0qh67OHXwPeXDDWmlxONQfHs9hQfrdiETVAw+8nZ0IE+8qrWtyPqrQqZ7OXa2ZYcBCg==

l0rinc

Concept ACK

l0rinc · 2025-09-01T18:34:03Z

ci/test/03_test_script.sh

@@ -33,7 +33,9 @@ echo "=== BEGIN env ==="
 env
 echo "=== END env ==="

-(
+if [ "$RUN_TIDY" != "true" ]; then


could we fix this in https://github.com/bitcoin-core/leveldb-subtree/pulls instead?
I have opened bitcoin-core/leveldb-subtree#56 (I'm also fine with doing it in a different PR, just wanted to give the possibility to solve it in a cleaner way)

No objection, but I am not sure we should diverge the subtree for a test-only patch. Also, the patch is pre-existing, so it seems a bit unrelated to this pull.

Ideally, Google would fix the UB in some way or another, and then we can pull whatever their fix was, but that shouldn't be a blocker here.

The main leveledb repo doesn't even compile for the past 8 months, not sure why we would wait for them. But as I mentioned, I don't mind doing it after the PR, just seems cleaner if we didn't need to change this section again.

l0rinc · 2025-09-01T18:40:15Z

src/index/base.h

 struct IndexSummary {
    std::string name;
    bool synced{false};
    int best_block_height{0};
    uint256 best_block_hash;
 };
+namespace interfaces {
+struct BlockRef;


not sure I understand why this was needed, but I haven't run the scripts, just tried to find out locally

fwd decls are slimmer than a full include, and often sufficient, for example when the header only uses the types as pointers or references. IWYU will suggest them sometimes, when possible.

I know, that wasn't my question. But I'll figure it out next, feel free to resolve it.

bitcoin/src/index/base.h

Line 128 in 1d49346

[[nodiscard]] virtual bool CustomInit(const std::optional<interfaces::BlockRef>& block) { return true; }

hebasto · 2025-09-04T18:03:26Z

@ryanofsky

You may be interested in these changes.

ryanofsky

Code review ACK 1d49346. Makes sense to try IWYU in a few directories like this to see how stable it is and make sure it won't cause problems.

ci/test/03_test_script.sh

hebasto · 2025-09-08T16:55:16Z

The feedback from @ryanofsky has been addressed.

ryanofsky

Code review ACK d03d861. Just fixing up comment and changing variable name since last review. Thanks for the updates!

ci/test/03_test_script.sh

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

hebasto · 2025-09-09T10:29:40Z

Rebased due to a conflict with #30469.

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

sipa · 2025-09-09T13:31:38Z

Concept ACK

ryanofsky

Code review ACK 02d2b5a. Just rebased and update tidy patch comment again since last review

hebasto added Refactoring Tests labels Nov 17, 2024

hebasto force-pushed the 241117-force-iwyu branch from 16dc941 to 2df21bd Compare November 18, 2024 00:15

DrahtBot added the CI failed label Nov 18, 2024

hebasto force-pushed the 241117-force-iwyu branch 2 times, most recently from 219d53e to fc855c6 Compare November 25, 2024 12:50

DrahtBot mentioned this pull request Nov 27, 2024

ci: Update Clang in "tidy" job #31306

Merged

DrahtBot added the Needs rebase label Dec 5, 2024

hebasto force-pushed the 241117-force-iwyu branch 2 times, most recently from 1646c06 to 2001fe8 Compare December 5, 2024 12:36

DrahtBot removed the Needs rebase label Dec 5, 2024

hebasto marked this pull request as ready for review December 5, 2024 13:16

DrahtBot added Needs rebase and removed CI failed labels Dec 5, 2024

hebasto force-pushed the 241117-force-iwyu branch from 2001fe8 to d838f1b Compare December 12, 2024 10:44

DrahtBot removed the Needs rebase label Dec 12, 2024

maflcko mentioned this pull request Dec 16, 2024

fuzz: Abort when using global PRNG without re-seed #31486

Merged

maflcko reviewed Jan 9, 2025

View reviewed changes

ci/test/03_test_script.sh Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 17, 2025

Safegcd-based modular inverses in MuHash3072 #21590

Merged

DrahtBot added the Needs rebase label Jan 27, 2025

maflcko mentioned this pull request Feb 5, 2025

lint: Qt exclusions are missing files #31801

Closed

DrahtBot removed the Needs rebase label Aug 14, 2025

DrahtBot added the CI failed label Sep 1, 2025

maflcko reviewed Sep 1, 2025

View reviewed changes

ci/test/03_test_script.sh Outdated Show resolved Hide resolved

hebasto force-pushed the 241117-force-iwyu branch from a4fa7b9 to 1d49346 Compare September 1, 2025 10:33

DrahtBot removed the CI failed label Sep 1, 2025

l0rinc reviewed Sep 1, 2025

View reviewed changes

DrahtBot mentioned this pull request Sep 6, 2025

index: Fix coinstats overflow #30469

Merged

ryanofsky approved these changes Sep 8, 2025

View reviewed changes

ci/test/03_test_script.sh Outdated Show resolved Hide resolved

ci/test/03_test_script.sh Outdated Show resolved Hide resolved

DrahtBot requested a review from l0rinc September 8, 2025 15:46

hebasto force-pushed the 241117-force-iwyu branch from 1d49346 to d03d861 Compare September 8, 2025 16:51

ryanofsky approved these changes Sep 8, 2025

View reviewed changes

ci/test/03_test_script.sh Outdated Show resolved Hide resolved

DrahtBot requested a review from maflcko September 8, 2025 17:27

hebasto force-pushed the 241117-force-iwyu branch from d03d861 to 9c8b8fe Compare September 8, 2025 17:32

DrahtBot added the Needs rebase label Sep 9, 2025

hebasto added 2 commits September 9, 2025 11:25

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

56f2a68

refactor: Fix includes in crypto directory

bdb8ead

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

hebasto force-pushed the 241117-force-iwyu branch from 9c8b8fe to 3a0456d Compare September 9, 2025 10:28

DrahtBot removed the Needs rebase label Sep 9, 2025

hebasto added 2 commits September 9, 2025 12:34

refactor: Fix includes in index directory

57a3eac

ci, iwyu: Treat warnings as errors for specific directories

02d2b5a

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

hebasto force-pushed the 241117-force-iwyu branch from 3a0456d to 02d2b5a Compare September 9, 2025 11:34

ryanofsky approved these changes Sep 9, 2025

View reviewed changes

DrahtBot requested a review from sipa September 9, 2025 13:36

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

Are you sure you want to change the base?

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

Conversation

hebasto commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

hebasto commented Dec 12, 2024

Uh oh!

Uh oh!

maflcko commented Sep 1, 2025

Uh oh!

Uh oh!

hebasto commented Sep 1, 2025

Uh oh!

maflcko commented Sep 1, 2025

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

l0rinc Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

l0rinc Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

l0rinc Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

l0rinc Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Sep 4, 2025

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hebasto commented Sep 8, 2025

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Sep 9, 2025

Uh oh!

sipa commented Sep 9, 2025

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Nov 17, 2024 •

edited

Loading

DrahtBot commented Nov 17, 2024 •

edited

Loading

l0rinc Sep 1, 2025 •

edited

Loading

hebasto Sep 2, 2025 •

edited

Loading