[PATCH] D156659: [clangd] Rollforward include-cleaner library usage in symbol collector.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:840
 
-    if (S->Scope == "std::" && S->Name == "move") {
-      if (!S->Signature.contains(','))
-        return "<utility>";
-      return "<algorithm>";
-    }
-   
-    if (auto StdSym = tooling::stdlib::Symbol::named(S->Scope, S->Name, Lang))
-     if (auto Header = StdSym->header())
-       return Header->name();
-    return "";
+  auto [It, Inserted] = SymbolProviders.try_emplace(S.ID);
+  auto Headers =
----------------
can you add a comment here saying, `We update providers for a symbol with each occurence, as SymbolCollector might run while parsing, rather than at the end of a translation unit. Hence we see more and more redecls over time.`


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:912
+    if (Directives & Symbol::Import) {
+      if (auto IncludeHeader = HeaderFileURIs->getIncludeHeader(FID);
+          !IncludeHeader.empty()) {
----------------
we should keep the `getStdHeaders` logic for objc


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:934
+        if (auto Canonical =
+                HeaderFileURIs->mapCanonical(H.physical()->getName());
+            !Canonical.empty())
----------------
can you add a `// FIXME: Get rid of this once include-cleaner has support for system headers.` to this branch


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:937
+          SpellingIt->second = Canonical;
+        else if (tooling::isSelfContainedHeader(H.physical(), SM,
+                                                PP->getHeaderSearchInfo()))
----------------
again a comment saying `For physical files, prefer URIs as spellings might change depending on the translation unit.`


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:127
+    if (FD->getNumParams() == 3 || FD->getNumParams() == 4)
       // move(InputIt first, InputIt last, OutputIt dest);
       return tooling::stdlib::Header::named("<algorithm>");
----------------
can you also add comment `move(ExecutionPolicy&& policy, ForwardIt1 first, ForwardIt1 last, ForwardIt2 d_first );` and move this change into a separate patch with a test case in clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp?


================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StdSpecialSymbolMap.inc:365
+// Remove them when the cause(s) are identified.
+SYMBOL(div, std::, <cstdlib>)
+SYMBOL(abort, std::, <cstdlib>)
----------------
can you move this into a separate patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156659