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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:471
+
+    // FIXME: this map should probably be in IncludeStructure.
+    llvm::StringMap<llvm::SmallVector<IncludeStructure::HeaderID>> BySpelling;
----------------
kadircet wrote:
> 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)
sure, I will address it in a followup patch.


================
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;
----------------
kadircet wrote:
> 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?
Yeah, this is a better solution, but I'm not sure whether we should do this before the release cut, it has a side effect of changing the find-macro-reference behavior in clangd. It requires some design/implement work.

I agree that the current solution is hacky, and eventually will be replaced, but it follows the existing `findReferencedMacros`, so it is not that bad. I tend to land this version before the release cut. What do you think?


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


================
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
----------------
kadircet wrote:
> 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`
> 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)

The IncludeStructure doesn't have a full support for IWYU export pragma, it only tracks the headers that have the export pragma.

My understand of the end goal is to use the `PragmaInclude` to handle every IWYU-related things, and we can remove all these IWYU bits in the `IncludeStructure`,  clangd IWYU pragma handling [code](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Headers.cpp#L127-L166).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875