[PATCH] D136071: [include-cleaner] WIP: Add PragmaIncludes which handles include-mapping pragmas.

[Sam McCall via Phabricator via cfe-commits]

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