[clang-tools-extra] r341463 - [clangd] Sort GoToDefinition results.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 5 05:00:16 PDT 2018
Author: hokein
Date: Wed Sep 5 05:00:15 2018
New Revision: 341463
URL: http://llvm.org/viewvc/llvm-project?rev=341463&view=rev
Log:
[clangd] Sort GoToDefinition results.
Summary:
GoToDefinition returns all declaration results (implicit/explicit) that are
in the same location, and the results are returned in arbitrary order.
Some LSP clients defaultly take the first result as the final result, which
might present a bad result (implicit decl) to users.
This patch ranks the result based on whether the declarations are
referenced explicitly/implicitly. We put explicit declarations first.
This also improves the "hover" (which just take the first result) feature
in some cases.
Reviewers: ilya-biryukov
Subscribers: kadircet, ioeric, MaskRay, jkorous, mgrang, arphaman, cfe-commits
Differential Revision: https://reviews.llvm.org/D50438
Modified:
clang-tools-extra/trunk/clangd/XRefs.cpp
clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
Modified: clang-tools-extra/trunk/clangd/XRefs.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/XRefs.cpp?rev=341463&r1=341462&r2=341463&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/XRefs.cpp (original)
+++ clang-tools-extra/trunk/clangd/XRefs.cpp Wed Sep 5 05:00:15 2018
@@ -69,10 +69,20 @@ struct MacroDecl {
const MacroInfo *Info;
};
+struct DeclInfo {
+ const Decl *D;
+ // Indicates the declaration is referenced by an explicit AST node.
+ bool IsReferencedExplicitly = false;
+};
+
/// Finds declarations locations that a given source location refers to.
class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
- std::vector<const Decl *> Decls;
std::vector<MacroDecl> MacroInfos;
+ // The value of the map indicates whether the declaration has been referenced
+ // explicitly in the code.
+ // True means the declaration is explicitly referenced at least once; false
+ // otherwise.
+ llvm::DenseMap<const Decl *, bool> Decls;
const SourceLocation &SearchedLocation;
const ASTContext &AST;
Preprocessor &PP;
@@ -82,13 +92,25 @@ public:
ASTContext &AST, Preprocessor &PP)
: SearchedLocation(SearchedLocation), AST(AST), PP(PP) {}
- std::vector<const Decl *> takeDecls() {
- // Don't keep the same declaration multiple times.
- // This can happen when nodes in the AST are visited twice.
- std::sort(Decls.begin(), Decls.end());
- auto Last = std::unique(Decls.begin(), Decls.end());
- Decls.erase(Last, Decls.end());
- return std::move(Decls);
+ // Get all DeclInfo of the found declarations.
+ // The results are sorted by "IsReferencedExplicitly" and declaration
+ // location.
+ std::vector<DeclInfo> getFoundDecls() const {
+ std::vector<DeclInfo> Result;
+ for (auto It : Decls) {
+ Result.emplace_back();
+ Result.back().D = It.first;
+ Result.back().IsReferencedExplicitly = It.second;
+ }
+
+ // Sort results. Declarations being referenced explicitly come first.
+ std::sort(Result.begin(), Result.end(),
+ [](const DeclInfo &L, const DeclInfo &R) {
+ if (L.IsReferencedExplicitly != R.IsReferencedExplicitly)
+ return L.IsReferencedExplicitly > R.IsReferencedExplicitly;
+ return L.D->getBeginLoc() < R.D->getBeginLoc();
+ });
+ return Result;
}
std::vector<MacroDecl> takeMacroInfos() {
@@ -112,15 +134,30 @@ public:
SourceLocation Loc,
index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
if (Loc == SearchedLocation) {
+ // Check whether the E has an implicit AST node (e.g. ImplicitCastExpr).
+ auto hasImplicitExpr = [](const Expr *E) {
+ if (!E || E->child_begin() == E->child_end())
+ return false;
+ // Use the first child is good enough for most cases -- normally the
+ // expression returned by handleDeclOccurence contains exactly one
+ // child expression.
+ const auto *FirstChild = *E->child_begin();
+ return llvm::isa<ExprWithCleanups>(FirstChild) ||
+ llvm::isa<MaterializeTemporaryExpr>(FirstChild) ||
+ llvm::isa<CXXBindTemporaryExpr>(FirstChild) ||
+ llvm::isa<ImplicitCastExpr>(FirstChild);
+ };
+
+ bool IsExplicit = !hasImplicitExpr(ASTNode.OrigE);
// Find and add definition declarations (for GoToDefinition).
// We don't use parameter `D`, as Parameter `D` is the canonical
// declaration, which is the first declaration of a redeclarable
// declaration, and it could be a forward declaration.
if (const auto *Def = getDefinition(D)) {
- Decls.push_back(Def);
+ Decls[Def] |= IsExplicit;
} else {
// Couldn't find a definition, fall back to use `D`.
- Decls.push_back(D);
+ Decls[D] |= IsExplicit;
}
}
return true;
@@ -158,7 +195,7 @@ private:
};
struct IdentifiedSymbol {
- std::vector<const Decl *> Decls;
+ std::vector<DeclInfo> Decls;
std::vector<MacroDecl> Macros;
};
@@ -172,7 +209,7 @@ IdentifiedSymbol getSymbolAtPosition(Par
indexTopLevelDecls(AST.getASTContext(), AST.getLocalTopLevelDecls(),
DeclMacrosFinder, IndexOpts);
- return {DeclMacrosFinder.takeDecls(), DeclMacrosFinder.takeMacroInfos()};
+ return {DeclMacrosFinder.getFoundDecls(), DeclMacrosFinder.takeMacroInfos()};
}
Range getTokenRange(ParsedAST &AST, SourceLocation TokLoc) {
@@ -250,10 +287,13 @@ std::vector<Location> findDefinitions(Pa
llvm::Optional<Location> Def;
llvm::Optional<Location> Decl;
};
- llvm::DenseMap<SymbolID, CandidateLocation> ResultCandidates;
+ // We respect the order in Symbols.Decls.
+ llvm::SmallVector<CandidateLocation, 8> ResultCandidates;
+ llvm::DenseMap<SymbolID, size_t> CandidatesIndex;
// Emit all symbol locations (declaration or definition) from AST.
- for (const auto *D : Symbols.Decls) {
+ for (const DeclInfo &DI : Symbols.Decls) {
+ const Decl *D = DI.D;
// Fake key for symbols don't have USR (no SymbolID).
// Ideally, there should be a USR for each identified symbols. Symbols
// without USR are rare and unimportant cases, we use the a fake holder to
@@ -262,7 +302,11 @@ std::vector<Location> findDefinitions(Pa
if (auto ID = getSymbolID(D))
Key = *ID;
- auto &Candidate = ResultCandidates[Key];
+ auto R = CandidatesIndex.try_emplace(Key, ResultCandidates.size());
+ if (R.second) // new entry
+ ResultCandidates.emplace_back();
+ auto &Candidate = ResultCandidates[R.first->second];
+
auto Loc = findNameLoc(D);
auto L = makeLocation(AST, Loc);
// The declaration in the identified symbols is a definition if possible
@@ -278,7 +322,7 @@ std::vector<Location> findDefinitions(Pa
if (Index) {
LookupRequest QueryRequest;
// Build request for index query, using SymbolID.
- for (auto It : ResultCandidates)
+ for (auto It : CandidatesIndex)
QueryRequest.IDs.insert(It.first);
std::string HintPath;
const FileEntry *FE =
@@ -286,22 +330,21 @@ std::vector<Location> findDefinitions(Pa
if (auto Path = getRealPath(FE, SourceMgr))
HintPath = *Path;
// Query the index and populate the empty slot.
- Index->lookup(
- QueryRequest, [&HintPath, &ResultCandidates](const Symbol &Sym) {
- auto It = ResultCandidates.find(Sym.ID);
- assert(It != ResultCandidates.end());
- auto &Value = It->second;
-
- if (!Value.Def)
- Value.Def = toLSPLocation(Sym.Definition, HintPath);
- if (!Value.Decl)
- Value.Decl = toLSPLocation(Sym.CanonicalDeclaration, HintPath);
- });
+ Index->lookup(QueryRequest, [&HintPath, &ResultCandidates,
+ &CandidatesIndex](const Symbol &Sym) {
+ auto It = CandidatesIndex.find(Sym.ID);
+ assert(It != CandidatesIndex.end());
+ auto &Value = ResultCandidates[It->second];
+
+ if (!Value.Def)
+ Value.Def = toLSPLocation(Sym.Definition, HintPath);
+ if (!Value.Decl)
+ Value.Decl = toLSPLocation(Sym.CanonicalDeclaration, HintPath);
+ });
}
// Populate the results, definition first.
- for (auto It : ResultCandidates) {
- const auto &Candidate = It.second;
+ for (const auto &Candidate : ResultCandidates) {
if (Candidate.Def)
Result.push_back(*Candidate.Def);
if (Candidate.Decl &&
@@ -383,7 +426,11 @@ std::vector<DocumentHighlight> findDocum
const SourceManager &SM = AST.getASTContext().getSourceManager();
auto Symbols = getSymbolAtPosition(
AST, getBeginningOfIdentifier(AST, Pos, SM.getMainFileID()));
- auto References = findRefs(Symbols.Decls, AST);
+ std::vector<const Decl *> TargetDecls;
+ for (const DeclInfo &DI : Symbols.Decls) {
+ TargetDecls.push_back(DI.D);
+ }
+ auto References = findRefs(TargetDecls, AST);
std::vector<DocumentHighlight> Result;
for (const auto &Ref : References) {
@@ -650,7 +697,7 @@ Optional<Hover> getHover(ParsedAST &AST,
return getHoverContents(Symbols.Macros[0].Name);
if (!Symbols.Decls.empty())
- return getHoverContents(Symbols.Decls[0]);
+ return getHoverContents(Symbols.Decls[0].D);
auto DeducedType = getDeducedType(AST, SourceLocationBeg);
if (DeducedType && !DeducedType->isNull())
@@ -671,9 +718,15 @@ std::vector<Location> findReferences(Par
auto Loc = getBeginningOfIdentifier(AST, Pos, SM.getMainFileID());
auto Symbols = getSymbolAtPosition(AST, Loc);
+ std::vector<const Decl *> TargetDecls;
+ for (const DeclInfo &DI : Symbols.Decls) {
+ if (DI.IsReferencedExplicitly)
+ TargetDecls.push_back(DI.D);
+ }
+
// We traverse the AST to find references in the main file.
// TODO: should we handle macros, too?
- auto MainFileRefs = findRefs(Symbols.Decls, AST);
+ auto MainFileRefs = findRefs(TargetDecls, AST);
for (const auto &Ref : MainFileRefs) {
Location Result;
Result.range = getTokenRange(AST, Ref.Loc);
@@ -685,7 +738,7 @@ std::vector<Location> findReferences(Par
if (!Index)
return Results;
RefsRequest Req;
- for (const Decl *D : Symbols.Decls) {
+ for (const Decl *D : TargetDecls) {
// Not all symbols can be referenced from outside (e.g. function-locals).
// TODO: we could skip TU-scoped symbols here (e.g. static functions) if
// we know this file isn't a header. The details might be tricky.
Modified: clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp?rev=341463&r1=341462&r2=341463&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp Wed Sep 5 05:00:15 2018
@@ -311,6 +311,47 @@ TEST(GoToDefinition, All) {
}
}
+TEST(GoToDefinition, Rank) {
+ auto T = Annotations(R"cpp(
+ struct $foo1[[Foo]] {
+ $foo2[[Foo]]();
+ $foo3[[Foo]](Foo&&);
+ $foo4[[Foo]](const char*);
+ };
+
+ Foo $f[[f]]();
+
+ void $g[[g]](Foo foo);
+
+ void call() {
+ const char* $str[[str]] = "123";
+ Foo a = $1^str;
+ Foo b = Foo($2^str);
+ Foo c = $3^f();
+ $4^g($5^f());
+ g($6^str);
+ }
+ )cpp");
+ auto AST = TestTU::withCode(T.code()).build();
+ EXPECT_THAT(findDefinitions(AST, T.point("1")),
+ ElementsAre(RangeIs(T.range("str")), RangeIs(T.range("foo4"))));
+ EXPECT_THAT(findDefinitions(AST, T.point("2")),
+ ElementsAre(RangeIs(T.range("str"))));
+ EXPECT_THAT(findDefinitions(AST, T.point("3")),
+ ElementsAre(RangeIs(T.range("f")), RangeIs(T.range("foo3"))));
+ EXPECT_THAT(findDefinitions(AST, T.point("4")),
+ ElementsAre(RangeIs(T.range("g"))));
+ EXPECT_THAT(findDefinitions(AST, T.point("5")),
+ ElementsAre(RangeIs(T.range("f")), RangeIs(T.range("foo3"))));
+
+ auto DefinitionAtPoint6 = findDefinitions(AST, T.point("6"));
+ EXPECT_EQ(3ul, DefinitionAtPoint6.size());
+ EXPECT_THAT(DefinitionAtPoint6, HasSubsequence(RangeIs(T.range("str")),
+ RangeIs(T.range("foo4"))));
+ EXPECT_THAT(DefinitionAtPoint6, HasSubsequence(RangeIs(T.range("str")),
+ RangeIs(T.range("foo3"))));
+}
+
TEST(GoToDefinition, RelPathsInCompileCommand) {
// The source is in "/clangd-test/src".
// We build in "/clangd-test/build".
More information about the cfe-commits
mailing list