[PATCH] D88885: [clangd] Disambiguate overloads of std::move for header insertion.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Up until now, we relied on matching the filename.
This depends on unstable details of libstdc++ and doesn't work well on other
stdlibs. Also we'd like to remove it (see D88204 <https://reviews.llvm.org/D88204>).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88885

Files:
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1280,10 +1280,27 @@
   Language.CPlusPlus = true;
   Includes.addSystemHeadersMapping(Language);
   CollectorOpts.Includes = &Includes;
-  runSymbolCollector("namespace std { class string {}; }", /*Main=*/"");
-  EXPECT_THAT(Symbols,
-              Contains(AllOf(QName("std::string"), DeclURI(TestHeaderURI),
-                             IncludeHeader("<string>"))));
+  runSymbolCollector(
+      R"cpp(
+      namespace std {
+        class string {};
+        // Move overloads have special handling.
+        template <typename T> T&& move(T&&);
+        template <typename I, typename O> O move(I, I, O);
+      }
+      )cpp",
+      /*Main=*/"");
+  for (const auto &S : Symbols)
+    llvm::errs() << S.Scope << S.Name << " in " << S.IncludeHeaders.size()
+                 << "\n";
+  EXPECT_THAT(
+      Symbols,
+      UnorderedElementsAre(
+          QName("std"),
+          AllOf(QName("std::string"), DeclURI(TestHeaderURI),
+                IncludeHeader("<string>")),
+          AllOf(Labeled("move(T &&)"), IncludeHeader("<utility>")),
+          AllOf(Labeled("move(I, I, O)"), IncludeHeader("<algorithm>"))));
 }
 
 TEST_F(SymbolCollectorTest, IWYUPragma) {
Index: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
+++ clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
@@ -36,9 +36,9 @@
   // Usual standard library symbols are mapped correctly.
   EXPECT_EQ("<vector>", CI.mapHeader("path/vector.h", "std::vector"));
   EXPECT_EQ("<cstdio>", CI.mapHeader("path/stdio.h", "std::printf"));
-  // std::move is ambiguous, currently mapped only based on path
-  EXPECT_EQ("<utility>", CI.mapHeader("libstdc++/bits/move.h", "std::move"));
-  EXPECT_EQ("path/utility.h", CI.mapHeader("path/utility.h", "std::move"));
+  // std::move is ambiguous, currently always mapped to <utility>
+  EXPECT_EQ("<utility>",
+            CI.mapHeader("libstdc++/bits/stl_algo.h", "std::move"));
   // Unknown std symbols aren't mapped.
   EXPECT_EQ("foo/bar.h", CI.mapHeader("foo/bar.h", "std::notathing"));
   // iosfwd declares some symbols it doesn't own.
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -560,6 +560,11 @@
       QName = S->Scope;
       QName.append(S->Name);
       if (auto Header = getIncludeHeader(QName, Entry.second)) {
+        // Hack: there are two std::move() overloads from different headers.
+        // CanonicalIncludes returns the common one-arg one from <utility>.
+        if (S->Name == "move" && S->Scope == "std::" &&
+            S->Signature.contains(','))
+          *Header = "<algorithm>";
         Symbol NewSym = *S;
         NewSym.IncludeHeaders.push_back({*Header, 1});
         Symbols.insert(NewSym);
Index: clang-tools-extra/clangd/index/CanonicalIncludes.cpp
===================================================================
--- clang-tools-extra/clangd/index/CanonicalIncludes.cpp
+++ clang-tools-extra/clangd/index/CanonicalIncludes.cpp
@@ -90,6 +90,8 @@
     static const auto *Symbols = new llvm::StringMap<llvm::StringRef>({
 #define SYMBOL(Name, NameSpace, Header) {#NameSpace #Name, #Header},
 #include "StdSymbolMap.inc"
+        // There are two std::move()s, this is by far the most common.
+        SYMBOL(move, std::, <utility>)
 #undef SYMBOL
     });
     StdSymbolMapping = Symbols;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D88885.296410.patch
Type: text/x-patch
Size: 3870 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201006/49bb395c/attachment.bin>