[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:250
     if (!MFI.HeaderID) {
       elog("File {0} not found.", MFI.Written);
       continue;
----------------
(sorry, I missed this before)

We should not be elogging this case, we're already emitting a diagnostic and it doesn't indicate anything wrong with clangd itself.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:256
+    if (!Used && !mayConsiderUnused(MFI, AST)) {
+      dlog("{0} is not a valid unused header and will be filtered out",
+           MFI.Written);
----------------
This seems pretty vague text (particularly: what does valid mean, and is it valid but used or invalid but unused). Maybe "{0} was not used, but is not eligible to be diagnosed as unused"?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:64
 /// Retrieves headers that are referenced from the main file but not used.
+/// System headers and inclusions of headers with no header guard are dropped.
 std::vector<const Inclusion *>
----------------
I'm slightly worried about these sorts of comments becoming stale, they're really just describing what we think "not used" means.

I'd replace it with something more general like "In unclear cases, headers are not marked as unused", but up to you


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695