hokein updated this revision to Diff 160544.
hokein marked 3 inline comments as done.
hokein added a comment.
Address review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50438
Files:
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===================================================================
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -311,6 +311,50 @@
}
}
+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".
Index: clangd/XRefs.cpp
===================================================================
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -68,10 +68,24 @@
const MacroInfo *Info;
};
+struct DeclInfo {
+ const Decl *D;
+ // Indicates the declaration is referenced by an explicit AST node.
+ bool IsReferencedExplicitly;
+ bool operator==(const DeclInfo &L) {
+ return std::tie(D, IsReferencedExplicitly) ==
+ std::tie(L.D, L.IsReferencedExplicitly);
+ }
+};
+
/// 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 +96,19 @@
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);
+ std::vector<DeclInfo> getDecls() const {
+ std::vector<DeclInfo> Result;
+ for (auto It : Decls)
+ Result.push_back({It.first, 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 < R.D;
+ });
+ return Result;
}
std::vector<MacroDecl> takeMacroInfos() {
@@ -112,15 +132,30 @@
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 +193,7 @@
};
struct IdentifiedSymbol {
- std::vector<const Decl *> Decls;
+ std::vector<DeclInfo> Decls;
std::vector<MacroDecl> Macros;
};
@@ -172,7 +207,7 @@
indexTopLevelDecls(AST.getASTContext(), AST.getLocalTopLevelDecls(),
DeclMacrosFinder, IndexOpts);
- return {DeclMacrosFinder.takeDecls(), DeclMacrosFinder.takeMacroInfos()};
+ return {DeclMacrosFinder.getDecls(), DeclMacrosFinder.takeMacroInfos()};
}
llvm::Optional<Location>
@@ -253,19 +288,26 @@
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 auto &DI : Symbols.Decls) {
+ const auto *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
// minimize the invasiveness of these cases.
SymbolID Key("");
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, SourceRange(Loc, Loc));
// The declaration in the identified symbols is a definition if possible
@@ -281,30 +323,29 @@
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 =
SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
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 &&
@@ -386,7 +427,9 @@
getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
- std::vector<const Decl *> SelectedDecls = Symbols.Decls;
+ std::vector<const Decl *> SelectedDecls;
+ for (const auto &DI : Symbols.Decls)
+ SelectedDecls.push_back(DI.D);
DocumentHighlightsFinder DocHighlightsFinder(
llvm::errs(), AST.getASTContext(), AST.getPreprocessor(), SelectedDecls);
@@ -651,7 +694,7 @@
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())
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
