iwyu: Document or remove some `pragma: export` and other improvements by hebasto · Pull Request #34639 · bitcoin/bitcoin

hebasto · 2026-02-20T17:54:01Z

This PR is a prerequisite for #34448. It was split into a separate PR to limit the scope and minimize potential merge conflicts.

The first commit improves the accuracy of IWYU suggestions within our heavily templated code. Note that, for now, the serialize.h header itself is excluded from IWYU inspection because it lacks a corresponding source file.

The remaining commits follow the Developer Notes guidance:

Use IWYU pragma: export very sparingly, as this enforces transitive inclusion of headers and undermines the specific purpose of IWYU.

DrahtBot · 2026-02-20T17:54:17Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	w0xlt, ryanofsky, maflcko

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34793 (threading: never require logging from sync.h by theuni)
#34778 (logging: rewrite macros to add ratelimit option, avoid unused strprintf, clarify confusing errors by ryanofsky)
#34448 (ci, iwyu: Fix warnings in src/util and treat them as errors by hebasto)
#34435 (refactor: use _MiB/_GiB consistently for byte conversions by l0rinc)
#34038 (logging: replace -loglevel with -trace, various API improvements by ajtowns)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#29256 (log, refactor: Allow log macros to accept context arguments by ryanofsky)
#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

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.

maflcko

Personally I think it is fine to use export for headers that are always (possibly with a single exception) simply forwarded via another header.

I think this applies to all cases here, except for the chrono one?

src/sync.h

src/util/time.h

src/util/log.h

hebasto · 2026-02-27T14:05:37Z

@maflcko

Thank you for the review! Your feedback has been addressed.

fanquake · 2026-02-27T14:10:02Z

src/util/log.h

 #ifndef BITCOIN_UTIL_LOG_H
 #define BITCOIN_UTIL_LOG_H

+// Export, as `logging/categories.h` is practically not used stand-alone.


is practically not used stand-alone.

Not sure I understand this comment. Why are we doing this, instead of including the things that we are using? Why does it matter if they might only be used when some other header is also used?

Also, has the rationale for when to do this (or not), been documented in the developer notes?

is practically not used stand-alone.

Not sure I understand this comment. Why are we doing this, instead of including the things that we are using? Why does it matter if they might only be used when some other header is also used?

I've reverted this change to its initial variant.

Also, has the rationale for when to do this (or not), been documented in the developer notes?

It seems reasonable to update the developer notes once we work through the entire codebase.

It seems reasonable to update the developer notes once we work through the entire codebase.

Why? The developer docs should reflect how we want new code to be written, and current code to be updated, otherwise you're just guaranteeing having to rework code again later on, rather than having it introduced/updated to how it should be, now.

If this is a blocker, maybe the dev notes can be updated to say that a comment with the rationale are required for exports?

It seems reasonable to update the developer notes once we work through the entire codebase.

Why? The developer docs should reflect how we want new code to be written, and current code to be updated, otherwise you're just guaranteeing having to rework code again later on, rather than having it introduced/updated to how it should be, now.

If this is a blocker, maybe the dev notes can be updated to say that a comment with the rationale are required for exports?

Thanks! Done.

hebasto · 2026-02-28T11:37:32Z

@theuni

Thank you for the review! Your feedback has been addressed.

maflcko · 2026-02-28T17:41:39Z

review ACK 7713575 🦆

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: review ACK 7713575cc83d44a9ffbd9d60ad34a3f49720139e 🦆
o4CFKfSlNayOksST4HbIfwGJA0virv5UET5Bo0O4srh70oXYZl6TmqhfNIvJrEgSZokTdwAo7DzOw9rtkFHnBA==

hebasto · 2026-03-04T14:32:11Z

@fanquake @maflcko @theuni

Thank you for the review. The most recent @fanquake's comment has been addressed.

theuni · 2026-03-04T15:35:32Z

I don't understand what makes that handful of .cpp files special? Why do only a few require a direct logging/categories.h include?

hebasto · 2026-03-04T15:46:48Z

I don't understand what makes that handful of .cpp files special? Why do only a few require a direct logging/categories.h include?

They are not special rather ones where IWYU warnings are treated as errors:

bitcoin/ci/test/03_test_script.sh

Line 186 in e09b816

    
           FILES_WITH_ENFORCED_IWYU="/src/((crypto|index|kernel|primitives|univalue/(lib|test)|zmq)/.*\\.cpp|node/blockstorage\\.cpp|node/utxo_snapshot\\.cpp|core_io\\.cpp|signet\\.cpp)"

It is assumed that this will be enforced for the entire codebase in the future.

maflcko · 2026-03-04T16:01:04Z

Only change is in the last commit.

review ACK 9791771 💧

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: review ACK 979177154a48388f1f771c65a135aca545099c75 💧
mcOxsGIg9Bf9IzlarH+8Z035ibkFUaV9qoTlMFanEmgSm+Zh42G/NGqvRYPOMReL6abMsbDLFDlkVqrojI57Aw==

theuni · 2026-03-04T16:26:31Z

I don't understand what makes that handful of .cpp files special? Why do only a few require a direct logging/categories.h include?

They are not special rather ones where IWYU warnings are treated as errors:

bitcoin/ci/test/03_test_script.sh

Line 186 in e09b816

FILES_WITH_ENFORCED_IWYU="/src/((crypto|index|kernel|primitives|univalue/(lib|test)|zmq)/.*\\.cpp|node/blockstorage\\.cpp|node/utxo_snapshot\\.cpp|core_io\\.cpp|signet\\.cpp)"

It is assumed that this will be enforced for the entire codebase in the future.

Sorry, but this progression just doesn't make sense to me. As @maflcko said, the categories are needed any time a log macro is used. Using both includes everywhere (eventually) would be redundant and confusing.

It seems the original intent of logging/categories.h was to move them out of logging.h so that the kernel can use them without our logger impl. That makes sense.

But fundamentally, Category, Level, and SourceLocation are tied together.

util/log.h is the home of Level, and SourceLocation. It's also barebones and leaves out the implementation details of the logger.

So it makes sense to me to either:

just move LogFlags into there directly and be done with it
pragma export logging/categories.h from util/log.h

I don't see any harm in keeping the enum in a separate header for cleanliness, so I'd prefer the latter.

ryanofsky

Code review ACK 9791771.

I agree with @theuni it could be a good idea to just document the export in the last commit 9791771 instead of removing it because right now the util/log.h header explicitly references BCLog::LogFlags::ALL, so it has a hard dependency on the categories.h include.

(We could break the util/log.h dependency on categories.h by defining Category as std::optional<uint64_t> instead of uint64_t and using nullopt to represent an unset category instead of BCLog::LogFlags::ALL. Or we could go the other direction and move the categories into util and drop the idea of having an opaque category field. @stickies-v introduced the opaque category field in 5b01eae from #34374 so I'm not actually sure about reasons it is opaque. But if seems like either the field should be opaque OR util/log.h should depend on the non-opaque enum, but not both.)

Including the missing `<span>` header in `serialize.h` allows IWYU to correctly evaluate its redundancy elsewhere.

This makes `threadsafety.h` agnostic to the actual implementation.

hebasto · 2026-03-10T18:09:25Z

Since most reviewers agreed to export <logging/categories.h> from <util/log.h>, we are moving forward with this approach.

Let me know if you have a specific alternative in mind for the inline comment.

w0xlt

ACK 6a27dcd

ryanofsky

Code review ACK 6a27dcd, just keeping the log.h categories export and documenting it which makes sense for now.

As mentioned previously in longer term I think it'd make sense to either (1) drop the log.h dependency on categories.h if category is supposed to be opaque within util (easy to do with std::optiona) or (2) move categories into util and drop the idea that categories should be opaque. The current middle ground doesn't seem to make a lot of sense. The util library should either know about category values or not know about them. It shouldn't pretend to not know about them and also directly reference one of the values it doesn't know about.

maflcko · 2026-03-10T18:23:46Z

Only change is the last commit, again

review ACK 6a27dcd 😌

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: review ACK 6a27dcd99701aa3d776b837d400776129b53a29e 😌
kXDWNUlHQ6UOufO/FEtKNbnhJ8IbuIu1jnVaNlymzYsHtv8MlhVbfwdsai6rZx/jEUevrhTD0JHiIzrBBfxoCA==

maflcko · 2026-03-10T18:35:58Z

src/sync_unlogged.h

+
+// This header declares threading primitives compatible with Clang
+// Thread Safety Analysis and provides appropriate annotation macros.
+#include <threadsafety.h> // IWYU pragma: export


nit: I wonder if src/sync_unlogged.h should not be moved into src/util/sync_unlogged.h, because it seems like a low-level utility. Same for threadsafety.h. No need to re-touch here, but the move should be trivial, because the files are now basically only used in two places.

hebasto · 2026-03-13T12:56:18Z

A commit updating Developer Notes has been added.

hebasto added the Refactoring label Feb 20, 2026

hebasto mentioned this pull request Feb 20, 2026

ci, iwyu: Fix warnings in src/util and treat them as errors #34448

Draft

DrahtBot added CI failed and removed CI failed labels Feb 20, 2026

This was referenced Feb 24, 2026

logging: replace -loglevel with -trace, expose trace logging via RPC #34038

Open

wallet, logging: Replace WalletLogPrintf() with LogInfo() #30343

Draft

log, refactor: Allow log macros to accept context arguments #29256

Draft

maflcko reviewed Feb 27, 2026

View reviewed changes

src/sync.h Outdated Show resolved Hide resolved

src/util/time.h Outdated Show resolved Hide resolved

src/util/log.h Outdated Show resolved Hide resolved

hebasto force-pushed the 260220-iwyu-pragma branch from e61ec11 to de4c286 Compare February 27, 2026 14:02

hebasto changed the title ~~iwyu: Remove some pragma: export and other improvements~~ iwyu: Document or remove some pragma: export and other improvements Feb 27, 2026

fanquake reviewed Feb 27, 2026

View reviewed changes

hebasto force-pushed the 260220-iwyu-pragma branch from de4c286 to 8a149c6 Compare February 28, 2026 11:36

hebasto force-pushed the 260220-iwyu-pragma branch from 8a149c6 to 7713575 Compare February 28, 2026 11:43

DrahtBot added the CI failed label Feb 28, 2026

This was referenced Mar 1, 2026

refactor: use _MiB/_GiB consistently for byte conversions #34435

Open

test: add end-to-end tests for CConnman and PeerManager #26812

Open

kernel, logging: Pass Logger instances to kernel objects #30342

Draft

DrahtBot removed the CI failed label Mar 3, 2026

hebasto force-pushed the 260220-iwyu-pragma branch from 7713575 to 9791771 Compare March 4, 2026 14:29

ryanofsky approved these changes Mar 9, 2026

View reviewed changes

DrahtBot mentioned this pull request Mar 10, 2026

logging: rewrite macros to add ratelimit option, avoid unused strprintf, clarify confusing errors #34778

Open

hebasto added 5 commits March 10, 2026 18:03

serialize: Add missing <span> header

c1c6174

Including the missing `<span>` header in `serialize.h` allows IWYU to correctly evaluate its redundancy elsewhere.

refactor: Move StdMutex to its own header

f6e4408

This makes `threadsafety.h` agnostic to the actual implementation.

iwyu, doc: Document IWYU pragma: export for <threadsafety.h>

d11b25f

iwyu, doc: Document IWYU pragma: export for <chrono>

a5b2b5d

iwyu, doc: Document IWYU pragma: export for <logging/categories.h>

6a27dcd

hebasto force-pushed the 260220-iwyu-pragma branch from 9791771 to 6a27dcd Compare March 10, 2026 18:04

w0xlt reviewed Mar 10, 2026

View reviewed changes

DrahtBot requested review from maflcko and ryanofsky March 10, 2026 18:11

ryanofsky approved these changes Mar 10, 2026

View reviewed changes

maflcko reviewed Mar 10, 2026

View reviewed changes

DrahtBot mentioned this pull request Mar 11, 2026

threading: never require logging from sync.h #34793

Closed

doc: Document rationale for using IWYU pragma: export

3858fe3

Conversation

hebasto commented Feb 20, 2026

Uh oh!

DrahtBot commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented Feb 27, 2026

Uh oh!

fanquake Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

fanquake Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

hebasto Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

fanquake Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

hebasto Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

hebasto commented Feb 28, 2026

Uh oh!

maflcko commented Feb 28, 2026

Uh oh!

hebasto commented Mar 4, 2026

Uh oh!

theuni commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Mar 4, 2026

Uh oh!

maflcko commented Mar 4, 2026

Uh oh!

theuni commented Mar 4, 2026

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 10, 2026

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • 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 commented Mar 10, 2026

Uh oh!

maflcko Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Feb 20, 2026 •

edited

Loading

theuni commented Mar 4, 2026 •

edited

Loading

ryanofsky left a comment •

edited

Loading