[clang-tools-extra] r354585 - [clangd] Only report explicitly typed symbols during code navigation
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 21 06:48:41 PST 2019
Author: kadircet
Date: Thu Feb 21 06:48:33 2019
New Revision: 354585
URL: http://llvm.org/viewvc/llvm-project?rev=354585&view=rev
Log:
[clangd] Only report explicitly typed symbols during code navigation
Summary:
Clangd was reporting implicit symbols, like results of implicit cast
expressions during code navigation, which is not desired. For example:
```
struct Foo{ Foo(int); };
void bar(Foo);
vod foo() {
int x;
bar(^x);
}
```
Performing a GoTo on the point specified by ^ would give two results one
pointing to line `int x` and the other for definition of `Foo(int);`
Reviewers: ilya-biryukov, sammccall
Subscribers: ioeric, MaskRay, jkorous, mgrang, arphaman, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D58495
Modified:
clang-tools-extra/trunk/clangd/XRefs.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolInfoTests.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=354585&r1=354584&r2=354585&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/XRefs.cpp (original)
+++ clang-tools-extra/trunk/clangd/XRefs.cpp Thu Feb 21 06:48:33 2019
@@ -110,20 +110,10 @@ 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<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;
+ llvm::DenseSet<const Decl *> Decls;
const SourceLocation &SearchedLocation;
const ASTContext &AST;
Preprocessor &PP;
@@ -133,22 +123,14 @@ public:
ASTContext &AST, Preprocessor &PP)
: SearchedLocation(SearchedLocation), AST(AST), PP(PP) {}
- // 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;
- }
+ // The results are sorted by declaration location.
+ std::vector<const Decl *> getFoundDecls() const {
+ std::vector<const Decl *> Result;
+ for (const Decl *D : Decls)
+ Result.push_back(D);
- // Sort results. Declarations being referenced explicitly come first.
- llvm::sort(Result, [](const DeclInfo &L, const DeclInfo &R) {
- if (L.IsReferencedExplicitly != R.IsReferencedExplicitly)
- return L.IsReferencedExplicitly > R.IsReferencedExplicitly;
- return L.D->getBeginLoc() < R.D->getBeginLoc();
+ llvm::sort(Result, [](const Decl *L, const Decl *R) {
+ return L->getBeginLoc() < R->getBeginLoc();
});
return Result;
}
@@ -180,21 +162,21 @@ public:
// clang) if it has an invalid paren/brace location, since such
// experssion is impossible to write down.
if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(E))
- return CtorExpr->getNumArgs() > 0 &&
- CtorExpr->getParenOrBraceRange().isInvalid();
+ return CtorExpr->getParenOrBraceRange().isInvalid();
return isa<ImplicitCastExpr>(E);
};
- bool IsExplicit = !IsImplicitExpr(ASTNode.OrigE);
+ if (IsImplicitExpr(ASTNode.OrigE))
+ return true;
// 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[Def] |= IsExplicit;
+ Decls.insert(Def);
} else {
// Couldn't find a definition, fall back to use `D`.
- Decls[D] |= IsExplicit;
+ Decls.insert(D);
}
}
return true;
@@ -232,7 +214,7 @@ private:
};
struct IdentifiedSymbol {
- std::vector<DeclInfo> Decls;
+ std::vector<const Decl *> Decls;
std::vector<MacroDecl> Macros;
};
@@ -329,8 +311,7 @@ std::vector<LocatedSymbol> locateSymbolA
llvm::DenseMap<SymbolID, size_t> ResultIndex;
// Emit all symbol locations (declaration or definition) from AST.
- for (const DeclInfo &DI : Symbols.Decls) {
- const Decl *D = DI.D;
+ for (const Decl *D : Symbols.Decls) {
auto Loc = makeLocation(AST, findNameLoc(D), *MainFilePath);
if (!Loc)
continue;
@@ -456,11 +437,7 @@ std::vector<DocumentHighlight> findDocum
const SourceManager &SM = AST.getASTContext().getSourceManager();
auto Symbols = getSymbolAtPosition(
AST, getBeginningOfIdentifier(AST, Pos, SM.getMainFileID()));
- std::vector<const Decl *> TargetDecls;
- for (const DeclInfo &DI : Symbols.Decls) {
- TargetDecls.push_back(DI.D);
- }
- auto References = findRefs(TargetDecls, AST);
+ auto References = findRefs(Symbols.Decls, AST);
std::vector<DocumentHighlight> Result;
for (const auto &Ref : References) {
@@ -714,7 +691,7 @@ llvm::Optional<Hover> getHover(ParsedAST
return getHoverContents(Symbols.Macros[0].Name);
if (!Symbols.Decls.empty())
- return getHoverContents(Symbols.Decls[0].D);
+ return getHoverContents(Symbols.Decls[0]);
auto DeducedType = getDeducedType(AST, SourceLocationBeg);
if (DeducedType && !DeducedType->isNull())
@@ -738,15 +715,9 @@ 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(TargetDecls, AST);
+ auto MainFileRefs = findRefs(Symbols.Decls, AST);
for (const auto &Ref : MainFileRefs) {
Location Result;
Result.range = getTokenRange(AST, Ref.Loc);
@@ -759,7 +730,7 @@ std::vector<Location> findReferences(Par
RefsRequest Req;
Req.Limit = Limit;
- for (const Decl *D : TargetDecls) {
+ for (const Decl *D : Symbols.Decls) {
// 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.
@@ -790,9 +761,9 @@ std::vector<SymbolDetails> getSymbolInfo
std::vector<SymbolDetails> Results;
- for (const auto &Sym : Symbols.Decls) {
+ for (const Decl *D : Symbols.Decls) {
SymbolDetails NewSymbol;
- if (const NamedDecl *ND = dyn_cast<NamedDecl>(Sym.D)) {
+ if (const NamedDecl *ND = dyn_cast<NamedDecl>(D)) {
std::string QName = printQualifiedName(*ND);
std::tie(NewSymbol.containerName, NewSymbol.name) =
splitQualifiedName(QName);
@@ -804,7 +775,7 @@ std::vector<SymbolDetails> getSymbolInfo
}
}
llvm::SmallString<32> USR;
- if (!index::generateUSRForDecl(Sym.D, USR)) {
+ if (!index::generateUSRForDecl(D, USR)) {
NewSymbol.USR = USR.str();
NewSymbol.ID = SymbolID(NewSymbol.USR);
}
Modified: clang-tools-extra/trunk/unittests/clangd/SymbolInfoTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/SymbolInfoTests.cpp?rev=354585&r1=354584&r2=354585&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/SymbolInfoTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/SymbolInfoTests.cpp Thu Feb 21 06:48:33 2019
@@ -180,12 +180,8 @@ TEST(SymbolInfoTests, All) {
func_baz1(f^f);
}
)cpp",
- {
- CreateExpectedSymbolDetails(
- "ff", "func_baz2", "c:TestTU.cpp at 218@F at func_baz2#@ff"),
- CreateExpectedSymbolDetails(
- "bar", "bar::", "c:@S at bar@F at bar#&1$@S at foo#"),
- }},
+ {CreateExpectedSymbolDetails(
+ "ff", "func_baz2", "c:TestTU.cpp at 218@F at func_baz2#@ff")}},
{
R"cpp( // Type reference - declaration
struct foo;
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=354585&r1=354584&r2=354585&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp Thu Feb 21 06:48:33 2019
@@ -406,6 +406,16 @@ TEST(LocateSymbol, All) {
double y = va^r<int>;
)cpp",
+
+ R"cpp(// No implicit constructors
+ class X {
+ X(X&& x) = default;
+ };
+ X [[makeX]]() {}
+ void foo() {
+ auto x = m^akeX();
+ }
+ )cpp",
};
for (const char *Test : Tests) {
Annotations T(Test);
@@ -453,20 +463,25 @@ TEST(LocateSymbol, Ambiguous) {
Foo c = $3^f();
$4^g($5^f());
g($6^str);
+ Foo ab$7^c;
+ Foo ab$8^cd("asdf");
+ Foo foox = Fo$9^o("asdf");
}
)cpp");
auto AST = TestTU::withCode(T.code()).build();
// Ordered assertions are deliberate: we expect a predictable order.
- EXPECT_THAT(locateSymbolAt(AST, T.point("1")),
- ElementsAre(Sym("str"), Sym("Foo")));
+ EXPECT_THAT(locateSymbolAt(AST, T.point("1")), ElementsAre(Sym("str")));
EXPECT_THAT(locateSymbolAt(AST, T.point("2")), ElementsAre(Sym("str")));
- EXPECT_THAT(locateSymbolAt(AST, T.point("3")),
- ElementsAre(Sym("f"), Sym("Foo")));
+ EXPECT_THAT(locateSymbolAt(AST, T.point("3")), ElementsAre(Sym("f")));
EXPECT_THAT(locateSymbolAt(AST, T.point("4")), ElementsAre(Sym("g")));
- EXPECT_THAT(locateSymbolAt(AST, T.point("5")),
- ElementsAre(Sym("f"), Sym("Foo")));
- EXPECT_THAT(locateSymbolAt(AST, T.point("6")),
- ElementsAre(Sym("str"), Sym("Foo"), Sym("Foo")));
+ EXPECT_THAT(locateSymbolAt(AST, T.point("5")), ElementsAre(Sym("f")));
+ EXPECT_THAT(locateSymbolAt(AST, T.point("6")), ElementsAre(Sym("str")));
+ EXPECT_THAT(locateSymbolAt(AST, T.point("7")), ElementsAre(Sym("abc")));
+ EXPECT_THAT(locateSymbolAt(AST, T.point("8")),
+ ElementsAre(Sym("Foo"), Sym("abcd")));
+ EXPECT_THAT(locateSymbolAt(AST, T.point("9")),
+ // First one is class definition, second is the constructor.
+ ElementsAre(Sym("Foo"), Sym("Foo")));
}
TEST(LocateSymbol, RelPathsInCompileCommand) {
More information about the cfe-commits
mailing list