[PATCH] D136723: [include-cleaner] Record main-file macro occurences and includes

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Left a few initial comments, it looks roughly good to me (for the macro-usage case, I might miss some historical context there, I think we come to an agreement that what this patch proposes is the designed behavior).



================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:75
+    //  - for a logical file like <vector>, we check Spelled
+    llvm::SmallVector<const Include *> match(Header H) const;
+
----------------
sammccall wrote:
> in the prototype I reimplemented this function in clangd, but I expect we can just reuse the RecordedIncludes class instead, copying from clangd's includes list is cheap.
> 
> (That's one argument for putting this in a different header, which I can do already if you prefer)
that's an interesting idea, but we need to do it carefully because of `FileEntry*`, the `Include` structure has a filed of `FileEntry*`, it is not feasible for clangd to propagate it (clangd's `Inclusion` doesn't have it, instead it uses a `HeaderID` which is based on the `fs::UniqueID` to identify a physical file).

But I think this is not something we should worry about at the moment, and the current interfaces look quite good.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:79
+    std::vector<Include> All;
+    llvm::StringMap<llvm::SmallVector<unsigned>> BySpelling;
+    llvm::DenseMap<const FileEntry *, llvm::SmallVector<unsigned>> ByFile;
----------------
nit: worth a comment mentioning the unsigned is the index of `All`.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:118
+  const FileEntry *Resolved = nullptr; // e.g. /path/to/c++/v1/vector
+  SourceLocation Location;             // of hash in #include <vector>
+  unsigned Line = 0;                   // 1-based line number for #include
----------------
nit: HashLoc seems clearer than `Location`.

And it looks like `Location` and `Line` feels somewhat redundant -- we can get the Line loc from the `Location` with `SourceManager`. But I think it is fair to hold both, Line is an important property of the include, which will probably widely used.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:159
+    #define Y X
+    int one = x;
+  )cpp");
----------------
why using a lower case `x` here?


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:167
+
+    int one = ^X;
+    int uno = $exp^LATE; // a ref in LATE's expansion
----------------
this is a redef error with the one defined in the header, I think it is not intended, rename it?


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:180
+  Inputs.ExtraFiles["header.h"] = Header.code();
+  Inputs.ErrorOK = true; // missing header
+  auto AST = build();
----------------
IIUC, ErrorOK is irrelevant, and should be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136723