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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:191
 
-// FIXME(kirillbobyrev): We currently do not support the umbrella headers.
-// Standard Library headers are typically umbrella headers, and system headers
-// are likely to be the Standard Library headers. Until we have a good support
-// for umbrella headers and Standard Library headers, don't warn about them.
-bool mayConsiderUnused(const Inclusion *Inc) {
-  return Inc->Written.front() != '<';
+bool mayConsiderUnused(const Inclusion *Inc, const IncludeStructure &Includes,
+                       FileManager &FM, HeaderSearch &HeaderInfo) {
----------------
this is a local helper only, just pass in ParsedAST instead of all the extra params?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:247
   for (const Inclusion &MFI : Structure.MainFileIncludes) {
     // FIXME: Skip includes that are not self-contained.
     if (!MFI.HeaderID) {
----------------
this is fixed now (though I think it would be better addressed ~here)


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:292
   auto Refs = findReferencedLocations(AST);
   auto ReferencedFiles = translateToHeaderIDs(findReferencedFiles(Refs, SM),
                                               AST.getIncludeStructure(), SM);
----------------
while here, pull out findReferencedFiles into its own var, and leave a FIXME to adjust the set of referenced files to account for non-self-contained headers whose symbols can be attributed to their include parents


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:311
   for (const auto *Inc : computeUnusedIncludes(AST)) {
-    if (!mayConsiderUnused(Inc))
+    if (!mayConsiderUnused(Inc, AST.getIncludeStructure(),
+                           AST.getSourceManager().getFileManager(),
----------------
Oops, I missed this in previous review.

Why are we first computing which includes are unused (including ineligible) and then filtering some out, rather than doing this check in the loop in computeUnused?


================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:1483
 
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
----------------
This can be now or later, but we can't/shouldn't jam every feature into one test case.
At some point we need to find a way to split this up into different cases, or at least not have many features that can only be tested at the diagnostic level.
(If we call mayConsiderUnused inside computeUnused as suggested above, this no longer has to be a diagnostic test)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695