[PATCH] D138219: [include-cleaner] Show includes matched by refs in HTML report.Demo: https://htmlpreview.github.io/?https://gist.githubusercontent.com/sam-mccall/ecee6869e37af3db28089b64d8dce806/raw/8736e64c45af411e2c2d72adaed2dfc4410a5b36/ASTTests.html%25202

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D138219#3946020 <https://reviews.llvm.org/D138219#3946020>, @sammccall wrote:

> (I do think the outstanding issues are not related to this patch, so this is ready for review)

Agree (sorry, I didn't mean we should address in this patch). We have found a few issues, I think we should record them in the issue tracker so that we don't lose them.



================
Comment at: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp:116
   FileID File;
+  const FileEntry *FE;
 
----------------
nit: the `FE` can be removed -- we have the FileID, we can retrieve it by `SM.getFileEntryByID(File)`


================
Comment at: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp:126
+    SmallVector<const Include *> Includes;
+    bool Satisfied = false;
   };
----------------
Worth to add a comment on the meaning of `Satisfied`.




================
Comment at: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp:160
+
+    for (const auto &H : T.Headers) {
+      T.Includes.append(Includes.match(H));
----------------
The current behaivor looks fine to me, but this is not the final state, right? We will revisit it when we have all the ranking/included-header bits in the library, if so, probably add a FIXME?


================
Comment at: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp:162
+      T.Includes.append(Includes.match(H));
+      if (H.kind() == Header::Physical && H.physical() == FE)
+        T.Satisfied = true;
----------------
a comment -- this is for the target symbol defined in the main file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138219