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

[Haojian Wu via Phabricator via cfe-commits]

hokein 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:
> 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?
My original plan was 1/2/3 (4 probably yes, as they mostly share the same thing).

As a personal preference, I like the name `PragmaIncludes` as it perfectly matches 1/2/3, but not 4; while the `HeaderQuirks` is probably a better umbrella, but it seems not quite self-contained. for now, I'd keep it as-is, but happy to change if you have a strong preference.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Hooks.h:44
+
+  bool shouldKeep(FileEntryRef File) const;
+  // Returns the mapping include for the given physical header file.
----------------
sammccall wrote:
> At first glance, this signature doesn't look right: `keep` applies to directives, not to files.
> e.g.
> ```
> #include "foo.h" // keep
> #include "foo.h"
> ```
> the second should be removed
> 
> (it's a silly example, but in the past we've confused ourselves into doing IO or complicated string matching code for things that are about spelling).
> 
> I'd suggest some representation like the line number (where the `#` appears, not where the comment is), though the details probably matter less than the concept.
right, good point!


================
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:
> 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.
Added comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/Hooks.cpp:24
+static llvm::Optional<StringRef> parseIWYUPragma(const char *Text) {
+  constexpr llvm::StringLiteral IWYUPragma = "// IWYU pragma: ";
+  if (strncmp(Text, IWYUPragma.data(), IWYUPragma.size()))
----------------
sammccall wrote:
> while we're adding this, we should probably support `/*` as well as `//`, it's allowed per spec (I didn't add it to clangd just to keep NFC)
added handling of `/*`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136071/new/

https://reviews.llvm.org/D136071