[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/Record.h:41
+// spelling header rather than the header directly defines the symbol.
+class PragmaIncludes {
+public:
----------------
kadircet wrote:
> 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).
yeah, the plan is that this class is a low-level piece, only providing the data extracted from pragmas, and we will build another layer on top of this class (this is my next step).



================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:54
+
+  class RecordPragma;
+
----------------
kadircet wrote:
> why do we need to expose this?
no needed, move it to private.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:67
+  llvm::DenseMap<llvm::sys::fs::UniqueID, std::string /*VerbatimSpelling*/>
+      IWYUPrivate;
+
----------------
sammccall wrote:
> nit: naming a member after the keys rather than the values is confusing, consider `IWYUPublic` or just `PublicMapping`?
rename to `IWYUPublic`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136071