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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo marked 19 inline comments as done.
VitaNuo added a comment.

Thank you!



================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:839
+  if (!Headers.empty())
+    SymbolProviders.insert({S.ID, Headers[0]});
 }
----------------
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.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:913
+    if (!Inserted)
+      continue;
+    switch (H.kind()) {
----------------
kadircet wrote:
> we shouldn't `continue` here, it just means we can directly use `SpellingIt` to figure out what to put into `NewSym.IncludeHeaders`.
> 
> maybe something like:
> 
> ```
> auto [SpellingIt, Inserted] = HeaderSpelling.try_emplace(H);
> if (Inserted)
>   SpellingIt-> second = getSpelling(H, *PP);
> if(!SpellingIt->second.empty()) {
>   Symbol NewSym = *S;
>   NewSym.IncludeHeaders.push_back({SpellingIt->second, 1, Directives});
>   Symbols.insert(NewSym);
> }
> ```
> 
> ```
> std::string getSpelling(const include_cleaner::Header &H, const Preprocessor &PP) {
>   if(H.kind() == Physical) {
>    // Check if we have a mapping for the header in system includes.
>    // FIXME: get rid of this once stdlib mapping covers these system headers too.
>    CanonicalIncludes CI;
>    CI.addSystemHeadersMapping(PP.getLangOpts());
>     if (auto Canonical = CI.mapHeader(H.physical()->getLastRef()); !Canonical.empty())
>         return Canonical;
>     if (!tooling::isSelfContainedHeader(...))
>         return "";
>   }
>   // Otherwise use default spelling strategies in include_cleaner.
>   return include_cleaner::spellHeader(H);
> }
> ```
As agreed offline, it is not easy to split out a spelling function, since `HeaderFileURIs` is a private member of the symbol collector, and we seem to still need to generate URIs for physical headers going forward.  


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:877
   // We delay this until end of TU so header guards are all resolved.
   for (const auto &[SID, FID] : IncludeFiles) {
     if (const Symbol *S = Symbols.find(SID)) {
----------------
kadircet wrote:
> VitaNuo wrote:
> > kadircet wrote:
> > > let's iterate over `SymbolProviders` instead, as `IncludeFiles` is a legacy struct now.
> > Not sure I'm following. Iterating over `SymbolProviders` means retrieving `include_cleaner::Header`s. How would I handle the entire Obj-C part then, without the FID of the include header? 
> we're joining `IncludeFiles` and `SymbolProviders`, as they have the same keys.
> 
> right now you're iterating over the keys in `IncludeFiles` and doing a lookup into `SymbolProviders` using that key to get providers.
> we can also do the iteration over `SymbolProviders` and do the lookup into `IncludeFiles` instead to find associated `FID`.
Ok, makes sense now, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152900