[clang-tools-extra] 638f0cf - [clangd] Be more explicit on testing the optional DefLoc in LocatedSymbol.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 31 05:35:11 PDT 2020
Author: Haojian Wu
Date: 2020-07-31T14:34:56+02:00
New Revision: 638f0cf565f2121151c32d7eb52a1de0e333d5f6
URL: https://github.com/llvm/llvm-project/commit/638f0cf565f2121151c32d7eb52a1de0e333d5f6
DIFF: https://github.com/llvm/llvm-project/commit/638f0cf565f2121151c32d7eb52a1de0e333d5f6.diff
LOG: [clangd] Be more explicit on testing the optional DefLoc in LocatedSymbol.
And also fix a bug where we may return a meaningless location.
Differential Revision: https://reviews.llvm.org/D84919
Added:
Modified:
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index cf747b607f4a..1fc89f3e0847 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -405,15 +405,17 @@ locateSymbolTextually(const SpelledWord &Word, ParsedAST &AST,
log("locateSymbolNamedTextuallyAt: {0}", MaybeDeclLoc.takeError());
return;
}
- Location DeclLoc = *MaybeDeclLoc;
- Location DefLoc;
+ LocatedSymbol Located;
+ Located.PreferredDeclaration = *MaybeDeclLoc;
+ Located.Name = (Sym.Name + Sym.TemplateSpecializationArgs).str();
if (Sym.Definition) {
auto MaybeDefLoc = indexToLSPLocation(Sym.Definition, MainFilePath);
if (!MaybeDefLoc) {
log("locateSymbolNamedTextuallyAt: {0}", MaybeDefLoc.takeError());
return;
}
- DefLoc = *MaybeDefLoc;
+ Located.PreferredDeclaration = *MaybeDefLoc;
+ Located.Definition = *MaybeDefLoc;
}
if (ScoredResults.size() >= 3) {
@@ -424,11 +426,6 @@ locateSymbolTextually(const SpelledWord &Word, ParsedAST &AST,
return;
}
- LocatedSymbol Located;
- Located.Name = (Sym.Name + Sym.TemplateSpecializationArgs).str();
- Located.PreferredDeclaration = bool(Sym.Definition) ? DefLoc : DeclLoc;
- Located.Definition = DefLoc;
-
SymbolQualitySignals Quality;
Quality.merge(Sym);
SymbolRelevanceSignals Relevance;
diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
index 0a8f85ed5317..c9c115fd19d8 100644
--- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -41,6 +41,7 @@ using ::testing::ElementsAre;
using ::testing::Eq;
using ::testing::IsEmpty;
using ::testing::Matcher;
+using ::testing::UnorderedElementsAre;
using ::testing::UnorderedElementsAreArray;
MATCHER_P2(FileRange, File, Range, "") {
@@ -264,19 +265,23 @@ MATCHER_P3(Sym, Name, Decl, DefOrNone, "") {
<< llvm::to_string(arg.PreferredDeclaration);
return false;
}
+ if (!Def && !arg.Definition)
+ return true;
if (Def && !arg.Definition) {
*result_listener << "Has no definition";
return false;
}
- if (Def && arg.Definition->range != *Def) {
+ if (!Def && arg.Definition) {
+ *result_listener << "Definition is " << llvm::to_string(arg.Definition);
+ return false;
+ }
+ if (arg.Definition->range != *Def) {
*result_listener << "Definition is " << llvm::to_string(arg.Definition);
return false;
}
return true;
}
-::testing::Matcher<LocatedSymbol> Sym(std::string Name, Range Decl) {
- return Sym(Name, Decl, llvm::None);
-}
+
MATCHER_P(Sym, Name, "") { return arg.Name == Name; }
MATCHER_P(RangeIs, R, "") { return arg.range == R; }
@@ -771,7 +776,7 @@ TEST(LocateSymbol, TextualSmoke) {
auto AST = TU.build();
auto Index = TU.index();
EXPECT_THAT(locateSymbolAt(AST, T.point(), Index.get()),
- ElementsAre(Sym("MyClass", T.range())));
+ ElementsAre(Sym("MyClass", T.range(), T.range())));
}
TEST(LocateSymbol, Textual) {
@@ -891,18 +896,20 @@ TEST(LocateSymbol, Ambiguous) {
// FIXME: Target the constructor as well.
EXPECT_THAT(locateSymbolAt(AST, T.point("9")), ElementsAre(Sym("Foo")));
EXPECT_THAT(locateSymbolAt(AST, T.point("10")),
- ElementsAre(Sym("Foo", T.range("ConstructorLoc"))));
+ ElementsAre(Sym("Foo", T.range("ConstructorLoc"), llvm::None)));
EXPECT_THAT(locateSymbolAt(AST, T.point("11")),
- ElementsAre(Sym("Foo", T.range("ConstructorLoc"))));
+ ElementsAre(Sym("Foo", T.range("ConstructorLoc"), llvm::None)));
// These assertions are unordered because the order comes from
// CXXRecordDecl::lookupDependentName() which doesn't appear to provide
// an order guarantee.
EXPECT_THAT(locateSymbolAt(AST, T.point("12")),
- UnorderedElementsAre(Sym("bar", T.range("NonstaticOverload1")),
- Sym("bar", T.range("NonstaticOverload2"))));
- EXPECT_THAT(locateSymbolAt(AST, T.point("13")),
- UnorderedElementsAre(Sym("baz", T.range("StaticOverload1")),
- Sym("baz", T.range("StaticOverload2"))));
+ UnorderedElementsAre(
+ Sym("bar", T.range("NonstaticOverload1"), llvm::None),
+ Sym("bar", T.range("NonstaticOverload2"), llvm::None)));
+ EXPECT_THAT(
+ locateSymbolAt(AST, T.point("13")),
+ UnorderedElementsAre(Sym("baz", T.range("StaticOverload1"), llvm::None),
+ Sym("baz", T.range("StaticOverload2"), llvm::None)));
}
TEST(LocateSymbol, TextualDependent) {
@@ -932,9 +939,10 @@ TEST(LocateSymbol, TextualDependent) {
// interaction between locateASTReferent() and
// locateSymbolNamedTextuallyAt().
auto Results = locateSymbolAt(AST, Source.point(), Index.get());
- EXPECT_THAT(Results, UnorderedElementsAre(
- Sym("uniqueMethodName", Header.range("FooLoc")),
- Sym("uniqueMethodName", Header.range("BarLoc"))));
+ EXPECT_THAT(Results,
+ UnorderedElementsAre(
+ Sym("uniqueMethodName", Header.range("FooLoc"), llvm::None),
+ Sym("uniqueMethodName", Header.range("BarLoc"), llvm::None)));
}
TEST(LocateSymbol, TemplateTypedefs) {
@@ -992,20 +1000,23 @@ int [[bar_not_preamble]];
auto Locations =
runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p1"));
EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
- EXPECT_THAT(*Locations, ElementsAre(Sym("foo", SourceAnnotations.range())));
+ EXPECT_THAT(*Locations, ElementsAre(Sym("foo", SourceAnnotations.range(),
+ SourceAnnotations.range())));
// Go to a definition in header_in_preamble.h.
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p2"));
EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
EXPECT_THAT(
*Locations,
- ElementsAre(Sym("bar_preamble", HeaderInPreambleAnnotations.range())));
+ ElementsAre(Sym("bar_preamble", HeaderInPreambleAnnotations.range(),
+ HeaderInPreambleAnnotations.range())));
// Go to a definition in header_not_in_preamble.h.
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p3"));
EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
EXPECT_THAT(*Locations,
ElementsAre(Sym("bar_not_preamble",
+ HeaderNotInPreambleAnnotations.range(),
HeaderNotInPreambleAnnotations.range())));
}
@@ -1039,21 +1050,25 @@ TEST(GoToInclude, All) {
// Test include in preamble.
auto Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point());
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
- EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
+ EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
+ HeaderAnnotations.range())));
// Test include in preamble, last char.
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("2"));
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
- EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
+ EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
+ HeaderAnnotations.range())));
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("3"));
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
- EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
+ EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
+ HeaderAnnotations.range())));
// Test include outside of preamble.
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("6"));
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
- EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
+ EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
+ HeaderAnnotations.range())));
// Test a few positions that do not result in Locations.
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("4"));
@@ -1062,11 +1077,13 @@ TEST(GoToInclude, All) {
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("5"));
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
- EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
+ EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
+ HeaderAnnotations.range())));
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("7"));
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
- EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
+ EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
+ HeaderAnnotations.range())));
// Objective C #import directive.
Annotations ObjC(R"objc(
@@ -1078,7 +1095,8 @@ TEST(GoToInclude, All) {
Server.addDocument(FooM, ObjC.code());
Locations = runLocateSymbolAt(Server, FooM, ObjC.point());
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
- EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
+ EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
+ HeaderAnnotations.range())));
}
TEST(LocateSymbol, WithPreamble) {
@@ -1103,7 +1121,7 @@ TEST(LocateSymbol, WithPreamble) {
// LocateSymbol goes to a #include file: the result comes from the preamble.
EXPECT_THAT(
cantFail(runLocateSymbolAt(Server, FooCpp, FooWithHeader.point())),
- ElementsAre(Sym("foo.h", FooHeader.range())));
+ ElementsAre(Sym("foo.h", FooHeader.range(), FooHeader.range())));
// Only preamble is built, and no AST is built in this request.
Server.addDocument(FooCpp, FooWithoutHeader.code(), "null",
@@ -1112,7 +1130,7 @@ TEST(LocateSymbol, WithPreamble) {
// stale one.
EXPECT_THAT(
cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
- ElementsAre(Sym("foo", FooWithoutHeader.range())));
+ ElementsAre(Sym("foo", FooWithoutHeader.range(), llvm::None)));
// Reset test environment.
runAddDocument(Server, FooCpp, FooWithHeader.code());
@@ -1122,7 +1140,7 @@ TEST(LocateSymbol, WithPreamble) {
// Use the AST being built in above request.
EXPECT_THAT(
cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
- ElementsAre(Sym("foo", FooWithoutHeader.range())));
+ ElementsAre(Sym("foo", FooWithoutHeader.range(), llvm::None)));
}
TEST(LocateSymbol, NearbyTokenSmoke) {
@@ -1133,7 +1151,7 @@ TEST(LocateSymbol, NearbyTokenSmoke) {
auto AST = TestTU::withCode(T.code()).build();
// We don't pass an index, so can't hit index-based fallback.
EXPECT_THAT(locateSymbolAt(AST, T.point()),
- ElementsAre(Sym("err", T.range())));
+ ElementsAre(Sym("err", T.range(), T.range())));
}
TEST(LocateSymbol, NearbyIdentifier) {
More information about the cfe-commits
mailing list