[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

regarding testing, i think updating all tests that we have in `IncludeCleanerTests` that call `computeUnusedIncludes` to also call the new function that'll use include-cleaner library should be enough.



================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:5
 
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../include-cleaner/include)
+
----------------
can you move this near the include_directories for clang-tidy below (near line 63)


================
Comment at: clang-tools-extra/clangd/Config.h:91
 
-  enum UnusedIncludesPolicy { Strict, None };
+  enum UnusedIncludesPolicy { Strict, None, Experiment };
   /// Controls warnings and errors when parsing code.
----------------
can you also add a comment saying that "`Experiment` tries to provide the same behaviour as `Strict` but using include-cleaner"


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:471
+
+    // FIXME: this map should probably be in IncludeStructure.
+    llvm::StringMap<llvm::SmallVector<IncludeStructure::HeaderID>> BySpelling;
----------------
agreed, let's change `StdlibHeaders` in `IncludeStructure` to actually be a `BySpelling` mapping, it should be no-op for existing implementation as we're only checking for stdlib headers already (and there's no other usage)


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:479
+    }
+    // FIXME: !!this is a hacky way to collect macro references.
+    std::vector<include_cleaner::SymbolReference> Macros;
----------------
this might behave slightly different than what we have in `RecordedPP`, and rest of the applications of include-cleaner will be using that. can we expose the pieces in RecordedPP that collects MacroRefs as a separate handler and attach that collector (combined with the skipped range collection inside `CollectMainFileMacros` and also still converting to `MainFileMacros` in the end (as we can't store sourcelocations/identifierinfos from preamble)?

afterwards we can use the names stored in there to get back to `IdentifierInfo`s and Ranges stored to get back to `SourceLocation`s. WDYT?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:487
+        Macros.push_back({Tok.location(),
+                          include_cleaner::Macro{/*Name=*/nullptr, DefLoc},
+                          include_cleaner::RefType::Explicit});
----------------
you can get the `IdentifierInfo` using `Name` inside `Macro` and PP.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:512
+        });
+    std::vector<const Inclusion *> Unused;
+    for (const auto &I : Includes.MainFileIncludes) {
----------------
let's use `getUnused` directly here, with an empty set of `PublicHeaders`.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:539
   const Config &Cfg = Config::current();
-  if (Cfg.Diagnostics.UnusedIncludes != Config::UnusedIncludesPolicy::Strict ||
+  if (Cfg.Diagnostics.UnusedIncludes == Config::UnusedIncludesPolicy::None ||
       Cfg.Diagnostics.SuppressAll ||
----------------
can you keep `computeUnusedIncludes` the same and introduce a new function that'll call `include_cleaner`? afterwards we can just do a switch on policy here and call the relevant function.


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:609
+      Config::UnusedIncludesPolicy::Experiment)
+    Pragmas.record(*Clang);
   // Copy over the macros in the preamble region of the main file, and combine
----------------
why do we need to collect pragmas in main file? i think we already have necessary information available via `IncludeStructure` (it stores keeps and exports, and we don't care about anything else in the main file AFAICT). so let's just use the PragmaIncludes we're getting from the Preamble directly? without even making a copy and returning a reference from the `Preamble` instead in `ParsedAST::getPragmaIncludes`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875