[PATCH] D155898: [clangd] Fix go-to-type target location
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rGd9d9a2cb2f0d: [clangd] Use index for go-to-type (authored by
sammccall).
Changed prior to commit:
https://reviews.llvm.org/D155898?vs=542703&id=543057#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155898/new/
https://reviews.llvm.org/D155898
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/XRefs.h
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -33,12 +33,10 @@
namespace {
using ::testing::AllOf;
-using ::testing::Contains;
using ::testing::ElementsAre;
using ::testing::Eq;
using ::testing::IsEmpty;
using ::testing::Matcher;
-using ::testing::Not;
using ::testing::UnorderedElementsAre;
using ::testing::UnorderedElementsAreArray;
using ::testing::UnorderedPointwise;
@@ -1879,7 +1877,7 @@
ASSERT_GT(A.points().size(), 0u) << Case;
for (auto Pos : A.points())
- EXPECT_THAT(findType(AST, Pos),
+ EXPECT_THAT(findType(AST, Pos, nullptr),
ElementsAre(
sym("Target", HeaderA.range("Target"), HeaderA.range("Target"
<< Case;
@@ -1892,7 +1890,7 @@
TU.Code = A.code().str();
ParsedAST AST = TU.build();
-EXPECT_THAT(findType(AST, A.point()),
+EXPECT_THAT(findType(AST, A.point(), nullptr),
UnorderedElementsAre(
sym("Target", HeaderA.range("Target"), HeaderA.range("Target")),
sym("smart_ptr", HeaderA.range("smart_ptr"), HeaderA.range("smart_ptr"))
@@ -1901,6 +1899,39 @@
}
}
+TEST(FindType, Definition) {
+ Annotations A(R"cpp(
+class $decl[[X]];
+X *^x;
+class $def[[X]] {};
+ )cpp");
+ auto TU = TestTU::withCode(A.code().str());
+ ParsedAST AST = TU.build();
+
+ EXPECT_THAT(findType(AST, A.point(), nullptr),
+ ElementsAre(sym("X", A.range("decl"), A.range("def";
+}
+
+TEST(FindType, Index) {
+ Annotations Def(R"cpp(
+// This definition is only available through the index.
+class [[X]] {};
+ )cpp");
+ TestTU DefTU = TestTU::withHeaderCode(Def.code());
+ DefTU.HeaderFilename = "def.h";
+ auto DefIdx = DefTU.index();
+
+ Annotations A(R"cpp(
+class [[X]];
+X *^x;
+ )cpp");
+ auto TU = TestTU::withCode(A.code().str());
+ ParsedAST AST = TU.build();
+
+ EXPECT_THAT(findType(AST, A.point(), DefIdx.get()),
+ ElementsAre(sym("X", A.range(), Def.range(;
+}
+
void checkFindRefs(llvm::StringRef Test, bool UseIndex = false) {
Annotations T(Test);
auto TU = TestTU::withCode(T.code());
Index: clang-tools-extra/clangd/XRefs.h
===
--- clang-tools-extra/clangd/XRefs.h
+++ clang-tools-extra/clangd/XRefs.h
@@ -107,7 +107,8 @@
///
/// For example, given `b^ar()` wher bar return Foo, this function returns the
/// definition of class Foo.
-std::vector findType(ParsedAST &AST, Position Pos);
+std::vector findType(ParsedAST &AST, Position Pos,
+const SymbolIndex *Index);
/// Returns references of the symbol at a specified \p Pos.
/// \p Limit limits the number of results returned (0 means no limit).
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -341,6 +341,50 @@
return Results;
}
+// Given LocatedSymbol results derived from the AST, query the index to obtain
+// definitions and preferred declarations.
+void enhanceLocatedSymbolsFromIndex(llvm::MutableArrayRef Result,
+const SymbolIndex *Index,
+llvm::StringRef MainFilePath) {
+ LookupRequest QueryRequest;
+ llvm::DenseMap ResultIndex;
+ for (unsigned I = 0; I < Result.size(); ++I) {
+if (auto ID = Result[I].ID) {
+ ResultIndex.try_emplace(ID, I);
+ QueryRequest.IDs.insert(ID);
+}
+ }
+ if (!Index || QueryRequest.IDs.empty())
+return;
+ std::string Scratch;
+ Index->lookup(QueryRequest, [&](const Symbol &Sym) {
+auto &R = Result[ResultIndex.lookup(Sym.ID)];
+
+if (R.Definition) { // from AST
+ // Special case: if the AST yielded a definition, then it may not be
+ // the right *declaration*. Prefer the one from the index.
+ if (auto Loc = toLSPLocation(Sym.CanonicalDeclaration, MainFilePath))
+R.PreferredDeclaration = *Loc;
+
+ // We might still prefer the definition from the index, e.g. for
+ // generated symbol
[PATCH] D155898: [clangd] Fix go-to-type target location
sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:347 +void enhanceLocatedSymbolsFromIndex( +llvm::MutableArrayRef Result, +const llvm::DenseMap &ResultIndex, usaxena95 wrote: > nit: ResultIndex looks like an implementation detail of this function and the > callers do not need it. SymbolID is already present as LocatedSymbol::ID so > we can compute this map in this function itself. Nice catch! We were in fact computing all the symbol ids twice, too.. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155898/new/ https://reviews.llvm.org/D155898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D155898: [clangd] Fix go-to-type target location
usaxena95 accepted this revision. usaxena95 added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/XRefs.cpp:347 +void enhanceLocatedSymbolsFromIndex( +llvm::MutableArrayRef Result, +const llvm::DenseMap &ResultIndex, nit: ResultIndex looks like an implementation detail of this function and the callers do not need it. SymbolID is already present as LocatedSymbol::ID so we can compute this map in this function itself. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155898/new/ https://reviews.llvm.org/D155898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D155898: [clangd] Fix go-to-type target location
sammccall created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, wangpc, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
Previously it'd ~always jump to the lexically first declaration, which was
often an unhelpful forward declaration.
- consult the index for definition and preferred declaration locations (query
code shared with go-to-definition)
- when unwrapping to LSP, prefer definition to declaration This matches
"go-to-definition", which is the most common go-to operation
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D155898
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/XRefs.h
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -33,12 +33,10 @@
namespace {
using ::testing::AllOf;
-using ::testing::Contains;
using ::testing::ElementsAre;
using ::testing::Eq;
using ::testing::IsEmpty;
using ::testing::Matcher;
-using ::testing::Not;
using ::testing::UnorderedElementsAre;
using ::testing::UnorderedElementsAreArray;
using ::testing::UnorderedPointwise;
@@ -1879,7 +1877,7 @@
ASSERT_GT(A.points().size(), 0u) << Case;
for (auto Pos : A.points())
- EXPECT_THAT(findType(AST, Pos),
+ EXPECT_THAT(findType(AST, Pos, nullptr),
ElementsAre(
sym("Target", HeaderA.range("Target"), HeaderA.range("Target"
<< Case;
@@ -1892,7 +1890,7 @@
TU.Code = A.code().str();
ParsedAST AST = TU.build();
-EXPECT_THAT(findType(AST, A.point()),
+EXPECT_THAT(findType(AST, A.point(), nullptr),
UnorderedElementsAre(
sym("Target", HeaderA.range("Target"), HeaderA.range("Target")),
sym("smart_ptr", HeaderA.range("smart_ptr"), HeaderA.range("smart_ptr"))
@@ -1901,6 +1899,39 @@
}
}
+TEST(FindType, Definition) {
+ Annotations A(R"cpp(
+class $decl[[X]];
+X *^x;
+class $def[[X]] {};
+ )cpp");
+ auto TU = TestTU::withCode(A.code().str());
+ ParsedAST AST = TU.build();
+
+ EXPECT_THAT(findType(AST, A.point(), nullptr),
+ ElementsAre(sym("X", A.range("decl"), A.range("def";
+}
+
+TEST(FindType, Index) {
+ Annotations Def(R"cpp(
+// This definition is only available through the index.
+class [[X]] {};
+ )cpp");
+ TestTU DefTU = TestTU::withHeaderCode(Def.code());
+ DefTU.HeaderFilename = "def.h";
+ auto DefIdx = DefTU.index();
+
+ Annotations A(R"cpp(
+class [[X]];
+X *^x;
+ )cpp");
+ auto TU = TestTU::withCode(A.code().str());
+ ParsedAST AST = TU.build();
+
+ EXPECT_THAT(findType(AST, A.point(), DefIdx.get()),
+ ElementsAre(sym("X", A.range(), Def.range(;
+}
+
void checkFindRefs(llvm::StringRef Test, bool UseIndex = false) {
Annotations T(Test);
auto TU = TestTU::withCode(T.code());
Index: clang-tools-extra/clangd/XRefs.h
===
--- clang-tools-extra/clangd/XRefs.h
+++ clang-tools-extra/clangd/XRefs.h
@@ -107,7 +107,8 @@
///
/// For example, given `b^ar()` wher bar return Foo, this function returns the
/// definition of class Foo.
-std::vector findType(ParsedAST &AST, Position Pos);
+std::vector findType(ParsedAST &AST, Position Pos,
+const SymbolIndex *Index);
/// Returns references of the symbol at a specified \p Pos.
/// \p Limit limits the number of results returned (0 means no limit).
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -341,6 +341,46 @@
return Results;
}
+// Given LocatedSymbol results derived from the AST, query the index to obtain
+// definitions and preferred declarations.
+void enhanceLocatedSymbolsFromIndex(
+llvm::MutableArrayRef Result,
+const llvm::DenseMap &ResultIndex,
+const SymbolIndex *Index, llvm::StringRef MainFilePath) {
+ if (!Index || ResultIndex.empty())
+return;
+ LookupRequest QueryRequest;
+ for (auto It : ResultIndex)
+QueryRequest.IDs.insert(It.first);
+ std::string Scratch;
+ Index->lookup(QueryRequest, [&](const Symbol &Sym) {
+auto &R = Result[ResultIndex.lookup(Sym.ID)];
+
+if (R.Definition) { // from AST
+ // Special case: if the AST yielded a definition, then it may not be
+ // the right *declaration*. Prefer the one from the index.
+ if (auto Loc = toLSPLocation(Sym.CanonicalDeclaration, MainFilePath))
+R.PreferredDeclaration = *Loc;
