[PATCH] D152900: [clangd] Update symbol collector to use include-cleaner.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks a lot for bearing with me, let's ship it!



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:91
+    const Inclusion &Inc, ParsedAST &AST,
+    std::shared_ptr<const include_cleaner::PragmaIncludes> PI) {
   // FIXME(kirillbobyrev): We currently do not support the umbrella headers.
----------------
nit: just `const include_cleaner::PragmaIncludes* PI`


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:260
+    // ensure the preprocessor has been set already.
+    SysHeaderMapping.addSystemHeadersMapping(PP->getLangOpts());
+    auto Canonical = SysHeaderMapping.mapHeader(HeaderPath);
----------------
`SysHeaderMapping` doesn't need to be part of class state, just define it here completely. (also gets rid of the need for extra explanation and any possible questions around what would happen when we add system headers multiple times)


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:427
 
+    llvm::StringRef Filename = FE->getName();
     if (!tooling::isSelfContainedHeader(*FE, PP->getSourceManager(),
----------------
nit: can you move this above the call to `mapCanonical` and use it there too?


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:832-833
+  const auto &SM = PP->getSourceManager();
   if (Opts.CollectIncludePath &&
-      shouldCollectIncludePath(S.SymInfo.Kind) != Symbol::Invalid)
+      shouldCollectIncludePath(S.SymInfo.Kind) != Symbol::Invalid) {
     // Use the expansion location to get the #include header since this is
----------------
nit: early exits might be more readable, as we've more nesting now. (`if (!Opts.Collect.. || shouldCollect..() == Symbol::Invalid) return;`)


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:878-881
+    const auto FIDIt = IncludeFiles.find(SID);
+    assert(FIDIt != IncludeFiles.end());
+
+    const auto FID = FIDIt->second;
----------------
nit: `auto FID = IncludeFiles.at(SID);`


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:931
+          SpellingIt->second =
+              HeaderFileURIs->toURI(H.physical()->tryGetRealPathName());
+      } else
----------------
i believe this should be just `H.physical()->getLastRef()` ? as we're passing in a fileentryref into `toURI`


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:932
+              HeaderFileURIs->toURI(H.physical()->tryGetRealPathName());
+      } else
+        SpellingIt->second = include_cleaner::spellHeader(
----------------
nit: braces, as we've multiple lines (despite being a single statement)


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:937-940
+    auto NewSym = *S;
+    if (!SpellingIt->second.empty())
+      NewSym.IncludeHeaders.push_back({SpellingIt->second, 1, Directives});
+    Symbols.insert(NewSym);
----------------
```
if (!SpellingIt->second.empty()) {
  auto NewSym = *S;
  NewSym.IncludeHeaders.push_back({SpellingIt->second, 1, Directives});
  Symbols.insert(NewSym);
}
```


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:839
+  if (!Headers.empty())
+    SymbolProviders.insert({S.ID, Headers[0]});
 }
----------------
VitaNuo wrote:
> kadircet wrote:
> > once we have the optional<Header> as the value type you can also rewrite this as:
> > ```
> > auto [It, Inserted] = SymbolProviders.try_emplace(S.ID);
> > if (Inserted) {
> >   auto Providers = include_cleaner::headersForSymbol(Sym, SM, Opts.PragmaIncludes.get());
> >   if(!Providers.empty()) It->second = Providers.front();
> > } 
> > ```
> > 
> > that way we can get rid of some redundant calls to `headersForSymbol`
> Sure, although I'm not sure where the redundant calls would be coming from. I've been under the impression that this function is supposed to be called once for each symbol.
yeah unfortunately `addDeclaration` can be called multiple times (with increasing declaration quality, e.g. once for the forward declaration, then for a complete definition of a class)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152900