sammccall added inline comments.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Hooks.h:38
+// spelling header rather than the header directly defines the symbol.
+class PragmaIncludes {
+public:
----------------
sammccall wrote:
> IWYU pragmas requires a PP listener, we'll also need PP events for other
> purposes (determining which files are self-contained, finding #includes in
> the main file). Should these be the same listener and write into the same
> class?
>
> The argument for fewer components is usability: the embedder (standalone
> tool, clangd, clang-tidy) is responsible for connecting these hooks to the
> compiler and feeding the results back into the analysis.
>
> So the library is somewhat simpler to use if the number of parts is small.
> We need (I think) at least one for the AST and one for the preprocessor,
> because they need to be connected at different parts of the lifecycle.
>
> How much do we gain by splitting up further than that?
After discussing offline: I think we expect this to be reused in clangd so
separating it from the bits that won't is useful.
I don't think the scope is exactly clear though: which of these should we
bundle together?
- IWYU pragmas related to headers (private, export)
- IWYU pragmas related to the main file (keep/no_include)
- other equivalent things like `#pragma include_instead`
- tracking whether headers are self-contained
- recording `#include` graph (not sure IWYU even needs this)
- tracking macro occurrences in the main file
- details of `#include` directives from the main file
I'm tempted to say 1/2/3/4 are in-scope (since they're all gathered from
roughly the same source and needed for the same thing) and so `PragmaIncludes`
seems a little too narrow. Suggest `HeaderQuirks` maybe?
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Hooks.h:53
+ // Main file headers that should be kept, decorated by "IWYU pragma: keep".
+ llvm::DenseSet<llvm::sys::fs::UniqueID> ShouldKeep; // FIXME: implement
+
----------------
sammccall wrote:
> Why are these UniqueID rather than FileEntry*? Is the idea to try to use this
> in clangd?
>
> We hadn't planned on doing this, because pushing clangd's weird requirements
> around not using FileEntry's to the library, ability to clone the data
> structure etc seemed pretty intrusive. Since we switched from name-based to
> UniqueID it might be doable...
After discussing offline, using UniqueID to ensure the results are usable
across FileManagers seems reasonable, but I think it needs to be documented.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136071/new/
https://reviews.llvm.org/D136071
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
