[clang-tools-extra] 69daa36 - [clangd] Disambiguate overloads of std::move for header insertion.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 7 10:42:48 PDT 2020
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) {
More information about the cfe-commits
mailing list