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;
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;
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits