-
Notifications
You must be signed in to change notification settings - Fork 547
Apply clangd IWYU export pragmas to transitive includes #1492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
kevinAlbs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were warnings discovered by hovering in VS code? Inspecting locally with clangd 21.1.5 shows more warnings in some files. Example:
I posted a sample. I can post more if helpful (maybe 30ish more?). But this might point to a different local set-up. I used VS code with clangd 21.1.5 and this configuration file:
CompileFlags:
CompilationDatabase: /Users/kevin.albertson/review/mongo-cxx-driver-1492/cmake-build
Diagnostics:
Includes:
AnalyzeAngledIncludes: true
IgnoreHeader:
- .*/_deps/.*AnalyzeAngledIncludes seemed needed to show most include warnings.
Regardless, this PR seems like an improvement and LGTM (even if there are not-yet-addressed warnings).
| #include <functional> | ||
| #include <type_traits> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #include <cstddef> | ||
| #include <cstdint> | ||
|
|
||
| #include <bsoncxx/array/view-fwd.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding export pragma on unused header. Since this is a v_noabi header, I expect the include should be kept to avoid a possibly breaking change.
| #include <bsoncxx/array/view-fwd.hpp> | |
| #include <bsoncxx/array/view-fwd.hpp> // IWYU pragma: export |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more appropriate pragma for this situation is actually keep rather than export:
It forces IWYU to keep an inclusion even if it is deemed unnecessary. [...] In this case,
std::vectorisn't used, so<vector>would normally be discarded, but the pragma instructs IWYU to leave it.
This way, IWYU will still direct users to include the relevant header even if a transitive include is present yet unused.
Applied to all relevant v_noabi public headers.
|
|
||
| #include <bsoncxx/v1/array/view.hpp> | ||
| #include <bsoncxx/v1/array/view.hpp> // IWYU pragma: export | ||
| #include <bsoncxx/v1/config/export.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eramongodb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AnalyzeAngledIncludesseemed needed to show most include warnings.
Overlooked this configuration option, thank you. Fixed all additional include-related warnings (missing-includes or unused-includes) in v1 components that I could find.
Inspecting locally with clangd 21.1.5 shows more warnings in some files.
Extended IWYU fixes (missing-includes and unused-includes) to v_noabi public headers. Unused includes in public v_noabi headers are given the "keep" pragma with a comment indicating its eventual removal.
| #include <cstddef> | ||
| #include <cstdint> | ||
|
|
||
| #include <bsoncxx/array/view-fwd.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more appropriate pragma for this situation is actually keep rather than export:
It forces IWYU to keep an inclusion even if it is deemed unnecessary. [...] In this case,
std::vectorisn't used, so<vector>would normally be discarded, but the pragma instructs IWYU to leave it.
This way, IWYU will still direct users to include the relevant header even if a transitive include is present yet unused.
Applied to all relevant v_noabi public headers.


To improve the quality of clangd's IWYU diagnostics, this PR applies the IWYU "export" macro to deliberate transitive includes.
Include directives which are "exported" are:
\par Includes), orAll clangd IWYU warnings in v1 components (or required by v1 components) are addressed. Reordering or removal of includes in v_noabi public headers are skipped to avoid unnecessary potential for breaking changes.
Even with these changes, the IWYU tool still emits noise, e.g.
<bsoncxx/private/bson.hh>exports<bson/bson.h>, but some components still emit the following warnings:despite other entities provided by
<bson/bson.h>being (correctly) handled by the export. The<bson/bson.h>might need to be annotated with similar comments for these warnings to go away. Similarly,<type_traits>does not seem to be identified via<bsoncxx/private/type_traits.hh>-><bsoncxx/v1/detail/type_traits.hpp>-><type_traits>either. (Possibly related: include-what-you-use/include-what-you-use#1645)Update: confirmed these are due to the lack of IWYU export pragmas in
<bson/bson.h>: mongodb/mongo-c-driver#2167.Fixing warnings for missing IWYU of third party library headers (e.g. Catch2 and
<catch2/catch_message.hpp>) are skipped by this PR.Unsure if there is a nice way to silence these warnings for headers we do not own.The following can be added to the clangd configuration file (either per-project or global):