[PATCH] D156403: [clangd] Revert the symbol collector behavior to old pre-include-cleaner-library behavior due to a regression.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:841-847
   auto [It, Inserted] = SymbolProviders.try_emplace(S.ID);
   if (Inserted) {
     auto Headers =
         include_cleaner::headersForSymbol(Sym, SM, Opts.PragmaIncludes);
     if (!Headers.empty())
       It->second = Headers.front();
   }
----------------
can you also get rid of this piece, and `SymbolProviders` map ?


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:873
   llvm::DenseMap<FileID, bool> FileToContainsImportsOrObjC;
   llvm::DenseMap<include_cleaner::Header, std::string> HeaderSpelling;
   // Fill in IncludeHeaders.
----------------
we can drop this too


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:903-927
+    // mapping inside the include-cleaner library again.
+    llvm::StringRef IncludeHeader;
+    if (auto StdSym = tooling::stdlib::Symbol::named(S->Scope, S->Name))
+      if (auto Header = StdSym->header())
+        IncludeHeader = Header->name();
+    if (S->Scope == "std::" && S->Name == "move") {
+      IncludeHeader = "<utility>";
----------------
rather than duplicating some of the checks, can we rewrite this block as:
```
llvm::StringRef IncludeHeader = getStdlibHeader(*S, ASTCtx->getLangOpts());
if (IncludeHeader.empty())
  IncludeHeader = HeaderFileURIs->getIncludeHeader(FID);
if (!IncludeHeader.empty()) {
      auto NewSym = *S;
      NewSym.IncludeHeaders.push_back({IncludeHeader, 1, Directives});
      Symbols.insert(NewSym);
}
```


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:905
+    llvm::StringRef IncludeHeader;
+    if (auto StdSym = tooling::stdlib::Symbol::named(S->Scope, S->Name))
+      if (auto Header = StdSym->header())
----------------
let's pass in the language options from ASTCtx into here


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:908-912
+    if (S->Scope == "std::" && S->Name == "move") {
+      IncludeHeader = "<utility>";
+      if (S->Signature.contains(','))
+        IncludeHeader = "<algorithm>";
+    }
----------------
i think the old order of, first dealing with `std::move` and then using `tooling::stdlib` is less error-prone and the order in which we apply the mapping in include-cleaner, so that would be a more compatible change going forward.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:929-964
+    // FIXME: use providers from include-cleaner library once it's polished
+    // for Objective-C.
+    continue;
+
+    // FIXME: Re-enable for C++ code. The code below uses include-cleaner
+    // library but is currently unreachable due to regression.
     assert(Directives == Symbol::Include);
----------------
let's drop this block to not create confusion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156403