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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:35
 
+// Captures #include mapping information. It analyses IWYU Pragma comments and
+// other use-instead-like mechanisms (#pragma include_instead) on included
----------------
triple slashes (all over the header)


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:41
+// spelling header rather than the header directly defines the symbol.
+class PragmaIncludes {
+public:
----------------
i think this interface is OK, if we're planning to implement another adapter on top of this class and use this only as a repository. Then have the adapter provide a more convenient interface/data structures for types of queries we'll have during `Location => Header` mapping inside include cleaner.

As those will most likely look like `getHeaders(SourceLocation)/getHeaders(string)` -> `vector<pair<Header, Signals>>`, without really caring about the details an implementation detail being mapped to some other file, or getting exports of a certain header (as discussed this is the piece that would require a little bit more detailed design).


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:49
+  // `IWYU pragma: keep`.
+  bool shouldKeep(unsigned HashLineNumber) const;
+  // Returns the mapping include for the given physical header file.
----------------
it feels a little bit unfortunate from layering perspective that this struct also needs to somehow have specialized knowledge about the main file, but also having the same comment handler logic both in here and inside a main file specific handler seems like too much duplication. So let's go with it in this way for now, but keep this quirk in mind so that we can decide on a better place in the future.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:54
+
+  class RecordPragma;
+
----------------
why do we need to expose this?


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:58
+  // Line numbers for the include directives in the main file that have the
+  // `IWYU pragma: keep` right after.
+  llvm::DenseSet</*0-indexed line number*/ unsigned> ShouldKeep;
----------------
we should also keep headers that're marked as `export`.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:64
+      // We always insert using the spelling from the pragma.
+      if (auto *FE = SM.getFileEntryForID(SM.getFileID(Range.getBegin())))
+        Out->IWYUPrivate.insert(
----------------
nit: we can keep track of the current FileID using `FileChanged/LexedFileChanged` events (also gets rid of the `isWrittenInMainFile` call above/below).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136071