[PATCH] D95450: [clangd] Respect ReferencesParams.context.includeDeclarations

[Sam McCall via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked 2 inline comments as done.
Closed by commit rGff4832dbff0c: [clangd] Respect 
ReferencesParams.context.includeDeclarations (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D95450?vs=319306=320472#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95450/new/

https://reviews.llvm.org/D95450

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/test/references.test
  clang-tools-extra/clangd/unittests/PreambleTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -37,6 +37,7 @@
 namespace clangd {
 namespace {
 
+using ::testing::AllOf;
 using ::testing::ElementsAre;
 using ::testing::Eq;
 using ::testing::IsEmpty;
@@ -290,7 +291,8 @@
 
 MATCHER_P(Sym, Name, "") { return arg.Name == Name; }
 
-MATCHER_P(RangeIs, R, "") { return arg.range == R; }
+MATCHER_P(RangeIs, R, "") { return arg.Loc.range == R; }
+MATCHER_P(AttrsAre, A, "") { return arg.Attributes == A; }
 
 TEST(LocateSymbol, WithIndex) {
   Annotations SymbolHeader(R"cpp(
@@ -1688,11 +1690,34 @@
   << Test;
 }
 
+void checkFindRefs(llvm::StringRef Test, bool UseIndex = false) {
+  Annotations T(Test);
+  auto TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  std::vector> ExpectedLocations;
+  for (const auto  : T.ranges())
+ExpectedLocations.push_back(AllOf(RangeIs(R), AttrsAre(0u)));
+  // $def is actually shorthand for both definition and declaration.
+  // If we have cases that are definition-only, we should change this.
+  for (const auto  : T.ranges("def"))
+ExpectedLocations.push_back(
+AllOf(RangeIs(R), AttrsAre(ReferencesResult::Definition |
+   ReferencesResult::Declaration)));
+  for (const auto  : T.ranges("decl"))
+ExpectedLocations.push_back(
+AllOf(RangeIs(R), AttrsAre(ReferencesResult::Declaration)));
+  EXPECT_THAT(
+  findReferences(AST, T.point(), 0, UseIndex ? TU.index().get() : nullptr)
+  .References,
+  UnorderedElementsAreArray(ExpectedLocations))
+  << Test;
+}
+
 TEST(FindReferences, WithinAST) {
   const char *Tests[] = {
   R"cpp(// Local variable
 int main() {
-  int [[foo]];
+  int $def[[foo]];
   [[^foo]] = 2;
   int test1 = [[foo]];
 }
@@ -1700,7 +1725,7 @@
 
   R"cpp(// Struct
 namespace ns1 {
-struct [[Foo]] {};
+struct $def[[Foo]] {};
 } // namespace ns1
 int main() {
   ns1::[[Fo^o]]* Params;
@@ -1708,15 +1733,15 @@
   )cpp",
 
   R"cpp(// Forward declaration
-class [[Foo]];
-class [[Foo]] {};
+class $decl[[Foo]];
+class $def[[Foo]] {};
 int main() {
   [[Fo^o]] foo;
 }
   )cpp",
 
   R"cpp(// Function
-int [[foo]](int) {}
+int $def[[foo]](int) {}
 int main() {
   auto *X = &[[^foo]];
   [[foo]](42);
@@ -1725,7 +1750,7 @@
 
   R"cpp(// Field
 struct Foo {
-  int [[foo]];
+  int $def[[foo]];
   Foo() : [[foo]](0) {}
 };
 int main() {
@@ -1735,8 +1760,8 @@
   )cpp",
 
   R"cpp(// Method call
-struct Foo { int [[foo]](); };
-int Foo::[[foo]]() {}
+struct Foo { int $decl[[foo]](); };
+int Foo::$def[[foo]]() {}
 int main() {
   Foo f;
   f.[[^foo]]();
@@ -1745,7 +1770,7 @@
 
   R"cpp(// Constructor
 struct Foo {
-  [[F^oo]](int);
+  $decl[[F^oo]](int);
 };
 void foo() {
   Foo f = [[Foo]](42);
@@ -1753,14 +1778,14 @@
   )cpp",
 
   R"cpp(// Typedef
-typedef int [[Foo]];
+typedef int $def[[Foo]];
 int main() {
   [[^Foo]] bar;
 }
   )cpp",
 
   R"cpp(// Namespace
-namespace [[ns]] {
+namespace $decl[[ns]] { // FIXME: def?
 struct Foo {};
 } // namespace ns
 int main() { [[^ns]]::Foo foo; }
@@ -1770,7 +1795,7 @@
 #define TYPE(X) X
 #define FOO Foo
 #define CAT(X, Y) X##Y
-class [[Fo^o]] {};
+class $def[[Fo^o]] {};
 void test() {
   TYPE([[Foo]]) foo;
   [[FOO]] foo2;
@@ -1780,7 +1805,7 @@
   )cpp",
 
   R"cpp(// Macros
-#define [[MA^CRO]](X) (X+1)
+#define $def[[MA^CRO]](X) (X+1)
 void test() {
   int x = 

[PATCH] D95450: [clangd] Respect ReferencesParams.context.includeDeclarations

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.h:86
+  enum ReferenceAttributes : unsigned {
+Plain = 0,
+Declaration = 1 << 0,

kbobyrev wrote:
> nit: `Plain` means neither `Declaration` nor `Definition`, right? Maybe 
> `None` then?
Haha, this is actually unused.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95450/new/

https://reviews.llvm.org/D95450

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95450: [clangd] Respect ReferencesParams.context.includeDeclarations

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev accepted this revision.
kbobyrev added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM with a couple of nits.




Comment at: clang-tools-extra/clangd/XRefs.h:23
 #include "clang/AST/Type.h"
+#include "clang/Basic/BitmaskEnum.h"
 #include "clang/Format/Format.h"

nit: are these two needed? I can't see any usages of symbols defined there.



Comment at: clang-tools-extra/clangd/XRefs.h:86
+  enum ReferenceAttributes : unsigned {
+Plain = 0,
+Declaration = 1 << 0,

nit: `Plain` means neither `Declaration` nor `Definition`, right? Maybe `None` 
then?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95450/new/

https://reviews.llvm.org/D95450

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95450: [clangd] Respect ReferencesParams.context.includeDeclarations

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kbobyrev.
Herald added subscribers: usaxena95, kadircet, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.

Unfortunately this treats overrides declarations as declarations, not as
references. I don't plan to land this until I have a fix for that issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95450

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/test/references.test
  clang-tools-extra/clangd/unittests/PreambleTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -37,6 +37,7 @@
 namespace clangd {
 namespace {
 
+using ::testing::AllOf;
 using ::testing::ElementsAre;
 using ::testing::Eq;
 using ::testing::IsEmpty;
@@ -290,7 +291,8 @@
 
 MATCHER_P(Sym, Name, "") { return arg.Name == Name; }
 
-MATCHER_P(RangeIs, R, "") { return arg.range == R; }
+MATCHER_P(RangeIs, R, "") { return arg.Loc.range == R; }
+MATCHER_P(AttrsAre, A, "") { return arg.Attributes == A; }
 
 TEST(LocateSymbol, WithIndex) {
   Annotations SymbolHeader(R"cpp(
@@ -1688,11 +1690,35 @@
   << Test;
 }
 
+void checkFindRefs(llvm::StringRef Test, bool UseIndex = false) {
+  Annotations T(Test);
+  auto TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  std::vector> ExpectedLocations;
+  for (const auto  : T.ranges())
+ExpectedLocations.push_back(
+AllOf(RangeIs(R), AttrsAre(ReferencesResult::Plain)));
+  // $def is actually shorthand for both definition and declaration.
+  // If we have cases that are definition-only, we should change this.
+  for (const auto  : T.ranges("def"))
+ExpectedLocations.push_back(
+AllOf(RangeIs(R), AttrsAre(ReferencesResult::Definition |
+   ReferencesResult::Declaration)));
+  for (const auto  : T.ranges("decl"))
+ExpectedLocations.push_back(
+AllOf(RangeIs(R), AttrsAre(ReferencesResult::Declaration)));
+  EXPECT_THAT(
+  findReferences(AST, T.point(), 0, UseIndex ? TU.index().get() : nullptr)
+  .References,
+  UnorderedElementsAreArray(ExpectedLocations))
+  << Test;
+}
+
 TEST(FindReferences, WithinAST) {
   const char *Tests[] = {
   R"cpp(// Local variable
 int main() {
-  int [[foo]];
+  int $def[[foo]];
   [[^foo]] = 2;
   int test1 = [[foo]];
 }
@@ -1700,7 +1726,7 @@
 
   R"cpp(// Struct
 namespace ns1 {
-struct [[Foo]] {};
+struct $def[[Foo]] {};
 } // namespace ns1
 int main() {
   ns1::[[Fo^o]]* Params;
@@ -1708,15 +1734,15 @@
   )cpp",
 
   R"cpp(// Forward declaration
-class [[Foo]];
-class [[Foo]] {};
+class $decl[[Foo]];
+class $def[[Foo]] {};
 int main() {
   [[Fo^o]] foo;
 }
   )cpp",
 
   R"cpp(// Function
-int [[foo]](int) {}
+int $def[[foo]](int) {}
 int main() {
   auto *X = &[[^foo]];
   [[foo]](42);
@@ -1725,7 +1751,7 @@
 
   R"cpp(// Field
 struct Foo {
-  int [[foo]];
+  int $def[[foo]];
   Foo() : [[foo]](0) {}
 };
 int main() {
@@ -1735,8 +1761,8 @@
   )cpp",
 
   R"cpp(// Method call
-struct Foo { int [[foo]](); };
-int Foo::[[foo]]() {}
+struct Foo { int $decl[[foo]](); };
+int Foo::$def[[foo]]() {}
 int main() {
   Foo f;
   f.[[^foo]]();
@@ -1745,7 +1771,7 @@
 
   R"cpp(// Constructor
 struct Foo {
-  [[F^oo]](int);
+  $decl[[F^oo]](int);
 };
 void foo() {
   Foo f = [[Foo]](42);
@@ -1753,14 +1779,14 @@
   )cpp",
 
   R"cpp(// Typedef
-typedef int [[Foo]];
+typedef int $def[[Foo]];
 int main() {
   [[^Foo]] bar;
 }
   )cpp",
 
   R"cpp(// Namespace
-namespace [[ns]] {
+namespace $decl[[ns]] { // FIXME: def?
 struct Foo {};
 } // namespace ns
 int main() { [[^ns]]::Foo foo; }
@@ -1770,7 +1796,7 @@
 #define TYPE(X) X
 #define FOO Foo
 #define CAT(X, Y) X##Y
-class [[Fo^o]] {};
+class $def[[Fo^o]] {};
 void test() {
   TYPE([[Foo]]) foo;
   [[FOO]] foo2;
@@ -1780,7 +1806,7 @@
   )cpp",
 
   R"cpp(// Macros
-#define [[MA^CRO]](X) (X+1)
+#define $def[[MA^CRO]](X) (X+1)
 void test() {
   int x =