Author: Sam McCall Date: 2020-10-07T19:42:41+02:00 New Revision: 69daa368cad34c3cff7e170d2a32652ce31ca9e5
URL: https://github.com/llvm/llvm-project/commit/69daa368cad34c3cff7e170d2a32652ce31ca9e5 DIFF: https://github.com/llvm/llvm-project/commit/69daa368cad34c3cff7e170d2a32652ce31ca9e5.diff LOG: [clangd] Disambiguate overloads of std::move for header insertion. 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). Differential Revision: https://reviews.llvm.org/D88885 Added: Modified: clang-tools-extra/clangd/index/CanonicalIncludes.cpp clang-tools-extra/clangd/index/SymbolCollector.cpp clang-tools-extra/clangd/index/SymbolCollector.h clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/index/CanonicalIncludes.cpp b/clang-tools-extra/clangd/index/CanonicalIncludes.cpp index 2822e359c0a5..120f121c7278 100644 --- a/clang-tools-extra/clangd/index/CanonicalIncludes.cpp +++ b/clang-tools-extra/clangd/index/CanonicalIncludes.cpp @@ -90,6 +90,8 @@ void CanonicalIncludes::addSystemHeadersMapping(const LangOptions &Language) { 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; diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp index 2e1f261ab18a..92ebd90e950c 100644 --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -557,11 +557,9 @@ void SymbolCollector::finish() { llvm::SmallString<256> QName; for (const auto &Entry : IncludeFiles) if (const Symbol *S = Symbols.find(Entry.first)) { - QName = S->Scope; - QName.append(S->Name); - if (auto Header = getIncludeHeader(QName, Entry.second)) { + if (auto Header = getIncludeHeader(*S, Entry.second)) { Symbol NewSym = *S; - NewSym.IncludeHeaders.push_back({*Header, 1}); + NewSym.IncludeHeaders.push_back({std::move(*Header), 1}); Symbols.insert(NewSym); } } @@ -736,8 +734,8 @@ void SymbolCollector::addDefinition(const NamedDecl &ND, /// Gets a canonical include (URI of the header or <header> or "header") for /// header of \p FID (which should usually be the *expansion* file). /// Returns None if includes should not be inserted for this file. -llvm::Optional<std::string> -SymbolCollector::getIncludeHeader(llvm::StringRef QName, FileID FID) { +llvm::Optional<std::string> SymbolCollector::getIncludeHeader(const Symbol &S, + FileID FID) { const SourceManager &SM = ASTCtx->getSourceManager(); const FileEntry *FE = SM.getFileEntryForID(FID); if (!FE || FE->getName().empty()) @@ -746,10 +744,18 @@ SymbolCollector::getIncludeHeader(llvm::StringRef QName, FileID FID) { // If a file is mapped by canonical headers, use that mapping, regardless // of whether it's an otherwise-good header (header guards etc). if (Opts.Includes) { + llvm::SmallString<256> QName = S.Scope; + QName.append(S.Name); llvm::StringRef Canonical = Opts.Includes->mapHeader(Filename, QName); // If we had a mapping, always use it. - if (Canonical.startswith("<") || Canonical.startswith("\"")) + if (Canonical.startswith("<") || Canonical.startswith("\"")) { + // Hack: there are two std::move() overloads from diff erent headers. + // CanonicalIncludes returns the common one-arg one from <utility>. + if (Canonical == "<utility>" && S.Name == "move" && + S.Signature.contains(',')) + Canonical = "<algorithm>"; return Canonical.str(); + } if (Canonical != Filename) return toURI(SM, Canonical, Opts); } @@ -757,7 +763,7 @@ SymbolCollector::getIncludeHeader(llvm::StringRef QName, FileID FID) { // A .inc or .def file is often included into a real header to define // symbols (e.g. LLVM tablegen files). if (Filename.endswith(".inc") || Filename.endswith(".def")) - return getIncludeHeader(QName, SM.getFileID(SM.getIncludeLoc(FID))); + return getIncludeHeader(S, SM.getFileID(SM.getIncludeLoc(FID))); // Conservatively refuse to insert #includes to files without guards. return llvm::None; } diff --git a/clang-tools-extra/clangd/index/SymbolCollector.h b/clang-tools-extra/clangd/index/SymbolCollector.h index 9b30aeba9538..a1b40a0dba79 100644 --- a/clang-tools-extra/clangd/index/SymbolCollector.h +++ b/clang-tools-extra/clangd/index/SymbolCollector.h @@ -131,7 +131,7 @@ class SymbolCollector : public index::IndexDataConsumer { void processRelations(const NamedDecl &ND, const SymbolID &ID, ArrayRef<index::SymbolRelation> Relations); - llvm::Optional<std::string> getIncludeHeader(llvm::StringRef QName, FileID); + llvm::Optional<std::string> getIncludeHeader(const Symbol &S, FileID); bool isSelfContainedHeader(FileID); // Heuristically headers that only want to be included via an umbrella. static bool isDontIncludeMeHeader(llvm::StringRef); diff --git a/clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp b/clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp index 7969b638d3d3..fa96a4579624 100644 --- a/clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp +++ b/clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp @@ -36,9 +36,9 @@ TEST(CanonicalIncludesTest, CXXStandardLibrary) { // 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. diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp index 80995baf946f..47071ac2da38 100644 --- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp +++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp @@ -1280,10 +1280,27 @@ TEST_F(SymbolCollectorTest, CanonicalSTLHeader) { 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) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
