[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