[PATCH] D89670: [clangd] Store the containing symbol for refs
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3bec07f91fb1: [clangd] Store the containing symbol for refs
(authored by nridge).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89670/new/
https://reviews.llvm.org/D89670
Files:
clang-tools-extra/clangd/index/Ref.h
clang-tools-extra/clangd/index/Serialization.cpp
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/index/SymbolCollector.h
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -70,21 +70,16 @@
MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
}
+bool rangesMatch(const SymbolLocation , const Range ) {
+ return std::make_tuple(Loc.Start.line(), Loc.Start.column(), Loc.End.line(),
+ Loc.End.column()) ==
+ std::make_tuple(R.start.line, R.start.character, R.end.line,
+ R.end.character);
+}
MATCHER_P(DeclRange, Pos, "") {
- return std::make_tuple(arg.CanonicalDeclaration.Start.line(),
- arg.CanonicalDeclaration.Start.column(),
- arg.CanonicalDeclaration.End.line(),
- arg.CanonicalDeclaration.End.column()) ==
- std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
- Pos.end.character);
-}
-MATCHER_P(DefRange, Pos, "") {
- return std::make_tuple(
- arg.Definition.Start.line(), arg.Definition.Start.column(),
- arg.Definition.End.line(), arg.Definition.End.column()) ==
- std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
- Pos.end.character);
+ return rangesMatch(arg.CanonicalDeclaration, Pos);
}
+MATCHER_P(DefRange, Pos, "") { return rangesMatch(arg.Definition, Pos); }
MATCHER_P(RefCount, R, "") { return int(arg.References) == R; }
MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
return static_cast(arg.Flags & Symbol::IndexedForCodeCompletion) ==
@@ -100,10 +95,7 @@
MATCHER(RefRange, "") {
const Ref = ::testing::get<0>(arg);
const Range = ::testing::get<1>(arg);
- return std::make_tuple(Pos.Location.Start.line(), Pos.Location.Start.column(),
- Pos.Location.End.line(), Pos.Location.End.column()) ==
- std::make_tuple(Range.start.line, Range.start.character,
- Range.end.line, Range.end.character);
+ return rangesMatch(Pos.Location, Range);
}
::testing::Matcher &>
HaveRanges(const std::vector Ranges) {
@@ -738,6 +730,76 @@
EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(Symbols, "FUNC").ID, _;
}
+TEST_F(SymbolCollectorTest, RefContainers) {
+ Annotations Code(R"cpp(
+int $toplevel1[[f1]](int);
+void f2() {
+ (void) $ref1a[[f1]](1);
+ auto fptr = &$ref1b[[f1]];
+}
+int $toplevel2[[v1]] = $ref2[[f1]](2);
+void f3(int arg = $ref3[[f1]](3));
+struct S1 {
+ int $classscope1[[member1]] = $ref4[[f1]](4);
+ int $classscope2[[member2]] = 42;
+};
+constexpr int f4(int x) { return x + 1; }
+template struct S2 {};
+S2<$ref6[[f4]](0)> v2;
+S2<$ref7a[[f4]](0)> f5(S2<$ref7b[[f4]](0)>);
+namespace N {
+ void $namespacescope1[[f6]]();
+ int $namespacescope2[[v3]];
+}
+ )cpp");
+ CollectorOpts.RefFilter = RefKind::All;
+ CollectorOpts.CollectMainFileRefs = true;
+ runSymbolCollector("", Code.code());
+ auto FindRefWithRange = [&](Range R) -> Optional {
+for (auto : Refs) {
+ for (auto : Entry.second) {
+if (rangesMatch(Ref.Location, R))
+ return Ref;
+ }
+}
+return llvm::None;
+ };
+ auto Container = [&](llvm::StringRef RangeName) {
+auto Ref = FindRefWithRange(Code.range(RangeName));
+EXPECT_TRUE(bool(Ref));
+return Ref->Container;
+ };
+ EXPECT_EQ(Container("ref1a"),
+findSymbol(Symbols, "f2").ID); // function body (call)
+ EXPECT_EQ(Container("ref1b"),
+findSymbol(Symbols, "f2").ID); // function body (address-of)
+ EXPECT_EQ(Container("ref2"),
+findSymbol(Symbols, "v1").ID); // variable initializer
+ EXPECT_EQ(Container("ref3"),
+findSymbol(Symbols, "f3").ID); // function parameter default value
+ EXPECT_EQ(Container("ref4"),
+findSymbol(Symbols, "S1::member1").ID); // member initializer
+ EXPECT_EQ(Container("ref5"),
+findSymbol(Symbols, "S2").ID); // template parameter default value
+ EXPECT_EQ(Container("ref6"),
+findSymbol(Symbols, "v2").ID); //
[PATCH] D89670: [clangd] Store the containing symbol for refs
nridge updated this revision to Diff 302759.
nridge marked 4 inline comments as done.
nridge added a comment.
Address final review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89670/new/
https://reviews.llvm.org/D89670
Files:
clang-tools-extra/clangd/index/Ref.h
clang-tools-extra/clangd/index/Serialization.cpp
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/index/SymbolCollector.h
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -70,21 +70,16 @@
MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
}
+bool rangesMatch(const SymbolLocation , const Range ) {
+ return std::make_tuple(Loc.Start.line(), Loc.Start.column(), Loc.End.line(),
+ Loc.End.column()) ==
+ std::make_tuple(R.start.line, R.start.character, R.end.line,
+ R.end.character);
+}
MATCHER_P(DeclRange, Pos, "") {
- return std::make_tuple(arg.CanonicalDeclaration.Start.line(),
- arg.CanonicalDeclaration.Start.column(),
- arg.CanonicalDeclaration.End.line(),
- arg.CanonicalDeclaration.End.column()) ==
- std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
- Pos.end.character);
-}
-MATCHER_P(DefRange, Pos, "") {
- return std::make_tuple(
- arg.Definition.Start.line(), arg.Definition.Start.column(),
- arg.Definition.End.line(), arg.Definition.End.column()) ==
- std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
- Pos.end.character);
+ return rangesMatch(arg.CanonicalDeclaration, Pos);
}
+MATCHER_P(DefRange, Pos, "") { return rangesMatch(arg.Definition, Pos); }
MATCHER_P(RefCount, R, "") { return int(arg.References) == R; }
MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
return static_cast(arg.Flags & Symbol::IndexedForCodeCompletion) ==
@@ -100,10 +95,7 @@
MATCHER(RefRange, "") {
const Ref = ::testing::get<0>(arg);
const Range = ::testing::get<1>(arg);
- return std::make_tuple(Pos.Location.Start.line(), Pos.Location.Start.column(),
- Pos.Location.End.line(), Pos.Location.End.column()) ==
- std::make_tuple(Range.start.line, Range.start.character,
- Range.end.line, Range.end.character);
+ return rangesMatch(Pos.Location, Range);
}
::testing::Matcher &>
HaveRanges(const std::vector Ranges) {
@@ -738,6 +730,76 @@
EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(Symbols, "FUNC").ID, _;
}
+TEST_F(SymbolCollectorTest, RefContainers) {
+ Annotations Code(R"cpp(
+int $toplevel1[[f1]](int);
+void f2() {
+ (void) $ref1a[[f1]](1);
+ auto fptr = &$ref1b[[f1]];
+}
+int $toplevel2[[v1]] = $ref2[[f1]](2);
+void f3(int arg = $ref3[[f1]](3));
+struct S1 {
+ int $classscope1[[member1]] = $ref4[[f1]](4);
+ int $classscope2[[member2]] = 42;
+};
+constexpr int f4(int x) { return x + 1; }
+template struct S2 {};
+S2<$ref6[[f4]](0)> v2;
+S2<$ref7a[[f4]](0)> f5(S2<$ref7b[[f4]](0)>);
+namespace N {
+ void $namespacescope1[[f6]]();
+ int $namespacescope2[[v3]];
+}
+ )cpp");
+ CollectorOpts.RefFilter = RefKind::All;
+ CollectorOpts.CollectMainFileRefs = true;
+ runSymbolCollector("", Code.code());
+ auto FindRefWithRange = [&](Range R) -> Optional {
+for (auto : Refs) {
+ for (auto : Entry.second) {
+if (rangesMatch(Ref.Location, R))
+ return Ref;
+ }
+}
+return llvm::None;
+ };
+ auto Container = [&](llvm::StringRef RangeName) {
+auto Ref = FindRefWithRange(Code.range(RangeName));
+EXPECT_TRUE(bool(Ref));
+return Ref->Container;
+ };
+ EXPECT_EQ(Container("ref1a"),
+findSymbol(Symbols, "f2").ID); // function body (call)
+ EXPECT_EQ(Container("ref1b"),
+findSymbol(Symbols, "f2").ID); // function body (address-of)
+ EXPECT_EQ(Container("ref2"),
+findSymbol(Symbols, "v1").ID); // variable initializer
+ EXPECT_EQ(Container("ref3"),
+findSymbol(Symbols, "f3").ID); // function parameter default value
+ EXPECT_EQ(Container("ref4"),
+findSymbol(Symbols, "S1::member1").ID); // member initializer
+ EXPECT_EQ(Container("ref5"),
+findSymbol(Symbols, "S2").ID); // template parameter default value
+ EXPECT_EQ(Container("ref6"),
+findSymbol(Symbols, "v2").ID); // type of variable
+ EXPECT_EQ(Container("ref7a"),
+findSymbol(Symbols,
[PATCH] D89670: [clangd] Store the containing symbol for refs
kadircet accepted this revision. kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:770 +EXPECT_TRUE(bool(Ref)); +if (!AllowNull) + EXPECT_FALSE(Ref->Container.isNull()); nit: instead of making this part of the lambda and complicating the signature I would just check this holds for classscope1 and namespacescope1 explicitly. it should be true for other cases anyways, as we are checking equality with non-null smybolids. Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:782 + +EXPECT_EQ(Ref1->Container, Ref2->Container); + }; nridge wrote: > kadircet wrote: > > can you also assert containers here are non-null (and below) > It looks like the container is null for toplevel decls. Is that a problem? I think that's OK for now, might be worth leaving a comment tho. (at `Ref::Container`) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89670/new/ https://reviews.llvm.org/D89670 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89670: [clangd] Store the containing symbol for refs
nridge added a comment.
(I will wait for a response about the containers for top-level decls before
committing.)
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:803
+
+ AssertSameContainer("toplevel1", "toplevel2");
+ AssertSameContainer("classscope1", "classscope2");
kadircet wrote:
> nit: i would suggest writing these in the form of:
>
> ```
> auto Container = [](StringRef RangeName){
> auto Ref = findRefWithRange(RangeName;
> ASSERT_TRUE(Ref);
> ASSERT_FALSE(Ref->Container.isNull());
> return Ref->Container;
> };
> EXPECT_EQ(Container("sym1"), Container("sym2"));
> EXPECT_NE(Container("sym1"), Container("sym2"));
> ```
>
> then you could update the ones above as:
> ```
> EXPECT_EQ(Container("rangename"), findSymbol(Symbols, "symname"));
> ```
Revised as suggested, except for allowing a null container for top-level decls.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89670/new/
https://reviews.llvm.org/D89670
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89670: [clangd] Store the containing symbol for refs
nridge updated this revision to Diff 302471.
nridge added a comment.
Address review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89670/new/
https://reviews.llvm.org/D89670
Files:
clang-tools-extra/clangd/index/Ref.h
clang-tools-extra/clangd/index/Serialization.cpp
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/index/SymbolCollector.h
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -70,21 +70,16 @@
MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
}
+bool rangesMatch(const SymbolLocation , const Range ) {
+ return std::make_tuple(Loc.Start.line(), Loc.Start.column(), Loc.End.line(),
+ Loc.End.column()) ==
+ std::make_tuple(R.start.line, R.start.character, R.end.line,
+ R.end.character);
+}
MATCHER_P(DeclRange, Pos, "") {
- return std::make_tuple(arg.CanonicalDeclaration.Start.line(),
- arg.CanonicalDeclaration.Start.column(),
- arg.CanonicalDeclaration.End.line(),
- arg.CanonicalDeclaration.End.column()) ==
- std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
- Pos.end.character);
-}
-MATCHER_P(DefRange, Pos, "") {
- return std::make_tuple(
- arg.Definition.Start.line(), arg.Definition.Start.column(),
- arg.Definition.End.line(), arg.Definition.End.column()) ==
- std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
- Pos.end.character);
+ return rangesMatch(arg.CanonicalDeclaration, Pos);
}
+MATCHER_P(DefRange, Pos, "") { return rangesMatch(arg.Definition, Pos); }
MATCHER_P(RefCount, R, "") { return int(arg.References) == R; }
MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
return static_cast(arg.Flags & Symbol::IndexedForCodeCompletion) ==
@@ -100,10 +95,7 @@
MATCHER(RefRange, "") {
const Ref = ::testing::get<0>(arg);
const Range = ::testing::get<1>(arg);
- return std::make_tuple(Pos.Location.Start.line(), Pos.Location.Start.column(),
- Pos.Location.End.line(), Pos.Location.End.column()) ==
- std::make_tuple(Range.start.line, Range.start.character,
- Range.end.line, Range.end.character);
+ return rangesMatch(Pos.Location, Range);
}
::testing::Matcher &>
HaveRanges(const std::vector Ranges) {
@@ -738,6 +730,78 @@
EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(Symbols, "FUNC").ID, _;
}
+TEST_F(SymbolCollectorTest, RefContainers) {
+ Annotations Code(R"cpp(
+int $toplevel1[[f1]](int);
+void f2() {
+ (void) $ref1a[[f1]](1);
+ auto fptr = &$ref1b[[f1]];
+}
+int $toplevel2[[v1]] = $ref2[[f1]](2);
+void f3(int arg = $ref3[[f1]](3));
+struct S1 {
+ int $classscope1[[member1]] = $ref4[[f1]](4);
+ int $classscope2[[member2]] = 42;
+};
+constexpr int f4(int x) { return x + 1; }
+template struct S2 {};
+S2<$ref6[[f4]](0)> v2;
+S2<$ref7a[[f4]](0)> f5(S2<$ref7b[[f4]](0)>);
+namespace N {
+ void $namespacescope1[[f6]]();
+ int $namespacescope2[[v3]];
+}
+ )cpp");
+ CollectorOpts.RefFilter = RefKind::All;
+ CollectorOpts.CollectMainFileRefs = true;
+ runSymbolCollector("", Code.code());
+ auto FindRefWithRange = [&](Range R) -> Optional {
+for (auto : Refs) {
+ for (auto : Entry.second) {
+if (rangesMatch(Ref.Location, R))
+ return Ref;
+ }
+}
+return llvm::None;
+ };
+ auto Container = [&](llvm::StringRef RangeName, bool AllowNull = false) {
+auto Ref = FindRefWithRange(Code.range(RangeName));
+EXPECT_TRUE(bool(Ref));
+if (!AllowNull)
+ EXPECT_FALSE(Ref->Container.isNull());
+return Ref->Container;
+ };
+ EXPECT_EQ(Container("ref1a"),
+findSymbol(Symbols, "f2").ID); // function body (call)
+ EXPECT_EQ(Container("ref1b"),
+findSymbol(Symbols, "f2").ID); // function body (address-of)
+ EXPECT_EQ(Container("ref2"),
+findSymbol(Symbols, "v1").ID); // variable initializer
+ EXPECT_EQ(Container("ref3"),
+findSymbol(Symbols, "f3").ID); // function parameter default value
+ EXPECT_EQ(Container("ref4"),
+findSymbol(Symbols, "S1::member1").ID); // member initializer
+ EXPECT_EQ(Container("ref5"),
+findSymbol(Symbols, "S2").ID); // template parameter default value
+ EXPECT_EQ(Container("ref6"),
+findSymbol(Symbols, "v2").ID); // type of variable
+
[PATCH] D89670: [clangd] Store the containing symbol for refs
nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:782 + +EXPECT_EQ(Ref1->Container, Ref2->Container); + }; kadircet wrote: > can you also assert containers here are non-null (and below) It looks like the container is null for toplevel decls. Is that a problem? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89670/new/ https://reviews.llvm.org/D89670 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89670: [clangd] Store the containing symbol for refs
kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, LGTM!
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:782
+
+EXPECT_EQ(Ref1->Container, Ref2->Container);
+ };
can you also assert containers here are non-null (and below)
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:803
+
+ AssertSameContainer("toplevel1", "toplevel2");
+ AssertSameContainer("classscope1", "classscope2");
nit: i would suggest writing these in the form of:
```
auto Container = [](StringRef RangeName){
auto Ref = findRefWithRange(RangeName;
ASSERT_TRUE(Ref);
ASSERT_FALSE(Ref->Container.isNull());
return Ref->Container;
};
EXPECT_EQ(Container("sym1"), Container("sym2"));
EXPECT_NE(Container("sym1"), Container("sym2"));
```
then you could update the ones above as:
```
EXPECT_EQ(Container("rangename"), findSymbol(Symbols, "symname"));
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89670/new/
https://reviews.llvm.org/D89670
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89670: [clangd] Store the containing symbol for refs
nridge updated this revision to Diff 302121.
nridge marked an inline comment as done.
nridge added a comment.
Address review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89670/new/
https://reviews.llvm.org/D89670
Files:
clang-tools-extra/clangd/index/Ref.h
clang-tools-extra/clangd/index/Serialization.cpp
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/index/SymbolCollector.h
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -70,21 +70,16 @@
MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
}
+bool rangesMatch(const SymbolLocation , const Range ) {
+ return std::make_tuple(Loc.Start.line(), Loc.Start.column(), Loc.End.line(),
+ Loc.End.column()) ==
+ std::make_tuple(R.start.line, R.start.character, R.end.line,
+ R.end.character);
+}
MATCHER_P(DeclRange, Pos, "") {
- return std::make_tuple(arg.CanonicalDeclaration.Start.line(),
- arg.CanonicalDeclaration.Start.column(),
- arg.CanonicalDeclaration.End.line(),
- arg.CanonicalDeclaration.End.column()) ==
- std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
- Pos.end.character);
-}
-MATCHER_P(DefRange, Pos, "") {
- return std::make_tuple(
- arg.Definition.Start.line(), arg.Definition.Start.column(),
- arg.Definition.End.line(), arg.Definition.End.column()) ==
- std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
- Pos.end.character);
+ return rangesMatch(arg.CanonicalDeclaration, Pos);
}
+MATCHER_P(DefRange, Pos, "") { return rangesMatch(arg.Definition, Pos); }
MATCHER_P(RefCount, R, "") { return int(arg.References) == R; }
MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
return static_cast(arg.Flags & Symbol::IndexedForCodeCompletion) ==
@@ -100,10 +95,7 @@
MATCHER(RefRange, "") {
const Ref = ::testing::get<0>(arg);
const Range = ::testing::get<1>(arg);
- return std::make_tuple(Pos.Location.Start.line(), Pos.Location.Start.column(),
- Pos.Location.End.line(), Pos.Location.End.column()) ==
- std::make_tuple(Range.start.line, Range.start.character,
- Range.end.line, Range.end.character);
+ return rangesMatch(Pos.Location, Range);
}
::testing::Matcher &>
HaveRanges(const std::vector Ranges) {
@@ -738,6 +730,85 @@
EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(Symbols, "FUNC").ID, _;
}
+TEST_F(SymbolCollectorTest, RefContainers) {
+ Annotations Code(R"cpp(
+int $toplevel1[[f1]](int);
+void f2() {
+ (void) $ref1a[[f1]](1);
+ auto fptr = &$ref1b[[f1]];
+}
+int $toplevel2[[v1]] = $ref2[[f1]](2);
+void f3(int arg = $ref3[[f1]](3));
+struct S1 {
+ int $classscope1[[member1]] = $ref4[[f1]](4);
+ int $classscope2[[member2]] = 42;
+};
+constexpr int f4(int x) { return x + 1; }
+template struct S2 {};
+S2<$ref6[[f4]](0)> v2;
+S2<$ref7a[[f4]](0)> f5(S2<$ref7b[[f4]](0)>);
+namespace N {
+ void $namespacescope1[[f6]]();
+ int $namespacescope2[[v3]];
+}
+ )cpp");
+ CollectorOpts.RefFilter = RefKind::All;
+ CollectorOpts.CollectMainFileRefs = true;
+ runSymbolCollector("", Code.code());
+ auto FindRefWithRange = [&](Range R) -> Optional {
+for (auto : Refs) {
+ for (auto : Entry.second) {
+if (rangesMatch(Ref.Location, R))
+ return Ref;
+ }
+}
+return llvm::None;
+ };
+ auto AssertContainer = [&](llvm::StringRef RefAnnotation,
+ llvm::StringRef ExpectedContainerName) {
+auto Ref = FindRefWithRange(Code.range(RefAnnotation));
+EXPECT_TRUE(bool(Ref));
+
+auto ExpectedContainer = findSymbol(Symbols, ExpectedContainerName);
+EXPECT_EQ(Ref->Container, ExpectedContainer.ID);
+ };
+ auto AssertSameContainer = [&](llvm::StringRef Ref1Ann,
+ llvm::StringRef Ref2Ann) {
+auto Ref1 = FindRefWithRange(Code.range(Ref1Ann));
+EXPECT_TRUE(bool(Ref1));
+auto Ref2 = FindRefWithRange(Code.range(Ref2Ann));
+EXPECT_TRUE(bool(Ref2));
+
+EXPECT_EQ(Ref1->Container, Ref2->Container);
+ };
+ auto AssertDifferentContainers = [&](llvm::StringRef Ref1Ann,
+ llvm::StringRef Ref2Ann) {
+auto Ref1 = FindRefWithRange(Code.range(Ref1Ann));
+EXPECT_TRUE(bool(Ref1));
+auto Ref2 = FindRefWithRange(Code.range(Ref2Ann));
+
[PATCH] D89670: [clangd] Store the containing symbol for refs
nridge added inline comments. Comment at: clang-tools-extra/clangd/index/Ref.h:91 RefKind Kind = RefKind::Unknown; + SymbolID Container; }; kadircet wrote: > i am afraid we are going to have an indeterminate value here if for whatever > reason `Container` detection fails. (e.g. for macros, or if ASTNode.Parent > set to nullptr by libindex). > > Sent out https://reviews.llvm.org/D90397 to address the issue. Good catch, and thank you for addressing this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89670/new/ https://reviews.llvm.org/D89670 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89670: [clangd] Store the containing symbol for refs
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/index/Ref.h:91
RefKind Kind = RefKind::Unknown;
+ SymbolID Container;
};
i am afraid we are going to have an indeterminate value here if for whatever
reason `Container` detection fails. (e.g. for macros, or if ASTNode.Parent set
to nullptr by libindex).
Sent out https://reviews.llvm.org/D90397 to address the issue.
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:779
+ AssertContainer("ref7b", "f5");// parameter type of function
+}
+
can we also have an assertion checking containers for top level symbols are the
same ?
and extend the test to have multiple declarations inside:
- a namespace
- inside a record (like S1)
and ensure the container is same for those (and different than top-level
symbols themselves). It is okay for those symbols.
Note that it is OK for namespaces to be not in the index, i just want to make
sure the containers for the top-level symbols inside a namespace are the same.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89670/new/
https://reviews.llvm.org/D89670
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89670: [clangd] Store the containing symbol for refs
nridge marked an inline comment as done.
nridge added inline comments.
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:349
+SymbolRef{SM.getFileLoc(Loc), Roles,
+ dyn_cast_or_null(ASTNode.Parent)});
// Don't continue indexing if this is a mere reference.
kadircet wrote:
> What does `ASTNode.Parent` correspond to in here? I couldn't find any
> clarifications on libIndex side, are we sure that's always what we want? It
> would be nice to have some tests demonstrating what this corresponds to in a
> variety of cases.
>
> Also why do we only store `NamedDecl`s as containers? It makes sense from
> CallHierarchy perspective as we are only interested in function-like
> containers, and they are nameddecls (?). But these might as well be
> `TranslationUnitDecl` (?) for top level declarations,
I wrote a test with various cases I could think of, and the choice of decl
coming from `ASTNode.Parent` seems reasonable to me.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89670/new/
https://reviews.llvm.org/D89670
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89670: [clangd] Store the containing symbol for refs
nridge updated this revision to Diff 300518.
nridge added a comment.
Use Decl rather than NamedDecl, and add a test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89670/new/
https://reviews.llvm.org/D89670
Files:
clang-tools-extra/clangd/index/Ref.h
clang-tools-extra/clangd/index/Serialization.cpp
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/index/SymbolCollector.h
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -70,21 +70,16 @@
MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
}
+bool rangesMatch(const SymbolLocation , const Range ) {
+ return std::make_tuple(Loc.Start.line(), Loc.Start.column(), Loc.End.line(),
+ Loc.End.column()) ==
+ std::make_tuple(R.start.line, R.start.character, R.end.line,
+ R.end.character);
+}
MATCHER_P(DeclRange, Pos, "") {
- return std::make_tuple(arg.CanonicalDeclaration.Start.line(),
- arg.CanonicalDeclaration.Start.column(),
- arg.CanonicalDeclaration.End.line(),
- arg.CanonicalDeclaration.End.column()) ==
- std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
- Pos.end.character);
-}
-MATCHER_P(DefRange, Pos, "") {
- return std::make_tuple(
- arg.Definition.Start.line(), arg.Definition.Start.column(),
- arg.Definition.End.line(), arg.Definition.End.column()) ==
- std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
- Pos.end.character);
+ return rangesMatch(arg.CanonicalDeclaration, Pos);
}
+MATCHER_P(DefRange, Pos, "") { return rangesMatch(arg.Definition, Pos); }
MATCHER_P(RefCount, R, "") { return int(arg.References) == R; }
MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
return static_cast(arg.Flags & Symbol::IndexedForCodeCompletion) ==
@@ -100,10 +95,7 @@
MATCHER(RefRange, "") {
const Ref = ::testing::get<0>(arg);
const Range = ::testing::get<1>(arg);
- return std::make_tuple(Pos.Location.Start.line(), Pos.Location.Start.column(),
- Pos.Location.End.line(), Pos.Location.End.column()) ==
- std::make_tuple(Range.start.line, Range.start.character,
- Range.end.line, Range.end.character);
+ return rangesMatch(Pos.Location, Range);
}
::testing::Matcher &>
HaveRanges(const std::vector Ranges) {
@@ -738,6 +730,54 @@
EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(Symbols, "FUNC").ID, _;
}
+TEST_F(SymbolCollectorTest, RefContainers) {
+ Annotations Code(R"cpp(
+int f1(int);
+void f2() {
+ (void) $ref1a[[f1]](1);
+ auto fptr = &$ref1b[[f1]];
+}
+int v1 = $ref2[[f1]](2);
+void f3(int arg = $ref3[[f1]](3));
+struct S1 {
+ int member = $ref4[[f1]](4);
+};
+constexpr int f4(int x) { return x + 1; }
+template struct S2 {};
+S2<$ref6[[f4]](0)> v2;
+S2<$ref7a[[f4]](0)> f5(S2<$ref7b[[f4]](0)>);
+ )cpp");
+ CollectorOpts.RefFilter = RefKind::All;
+ CollectorOpts.CollectMainFileRefs = true;
+ runSymbolCollector("", Code.code());
+ auto FindRefWithRange = [&](Range R) -> Optional {
+for (auto : Refs) {
+ for (auto : Entry.second) {
+if (rangesMatch(Ref.Location, R))
+ return Ref;
+ }
+}
+return llvm::None;
+ };
+ auto AssertContainer = [&](llvm::StringRef RefAnnotation,
+ llvm::StringRef ExpectedContainerName) {
+auto Ref = FindRefWithRange(Code.range(RefAnnotation));
+EXPECT_TRUE(bool(Ref));
+
+auto ExpectedContainer = findSymbol(Symbols, ExpectedContainerName);
+EXPECT_EQ(Ref->Container, ExpectedContainer.ID);
+ };
+ AssertContainer("ref1a", "f2");// function body (call)
+ AssertContainer("ref1b", "f2");// function body (address-of)
+ AssertContainer("ref2", "v1"); // variable initializer
+ AssertContainer("ref3", "f3"); // function parameter default value
+ AssertContainer("ref4", "S1::member"); // member initializer
+ AssertContainer("ref5", "S2"); // template parameter default value
+ AssertContainer("ref6", "v2"); // type of variable
+ AssertContainer("ref7a", "f5");// return type of function
+ AssertContainer("ref7b", "f5");// parameter type of function
+}
+
TEST_F(SymbolCollectorTest, MacroRefInHeader) {
Annotations Header(R"(
#define $foo[[FOO]](X) (X + 1)
Index: clang-tools-extra/clangd/index/SymbolCollector.h
[PATCH] D89670: [clangd] Store the containing symbol for refs
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:349
+SymbolRef{SM.getFileLoc(Loc), Roles,
+ dyn_cast_or_null(ASTNode.Parent)});
// Don't continue indexing if this is a mere reference.
What does `ASTNode.Parent` correspond to in here? I couldn't find any
clarifications on libIndex side, are we sure that's always what we want? It
would be nice to have some tests demonstrating what this corresponds to in a
variety of cases.
Also why do we only store `NamedDecl`s as containers? It makes sense from
CallHierarchy perspective as we are only interested in function-like
containers, and they are nameddecls (?). But these might as well be
`TranslationUnitDecl` (?) for top level declarations,
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89670/new/
https://reviews.llvm.org/D89670
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89670: [clangd] Store the containing symbol for refs
nridge created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
nridge requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
This is needed to implement call hierarchy.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D89670
Files:
clang-tools-extra/clangd/index/Ref.h
clang-tools-extra/clangd/index/Serialization.cpp
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/index/SymbolCollector.h
Index: clang-tools-extra/clangd/index/SymbolCollector.h
===
--- clang-tools-extra/clangd/index/SymbolCollector.h
+++ clang-tools-extra/clangd/index/SymbolCollector.h
@@ -156,7 +156,11 @@
std::shared_ptr CompletionAllocator;
std::unique_ptr CompletionTUInfo;
Options Opts;
- using SymbolRef = std::pair;
+ struct SymbolRef {
+SourceLocation Loc;
+index::SymbolRoleSet Roles;
+const NamedDecl *Container;
+ };
// Symbols referenced from the current TU, flushed on finish().
llvm::DenseSet ReferencedDecls;
llvm::DenseSet ReferencedMacros;
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -344,7 +344,9 @@
!isa(ND) &&
(Opts.RefsInHeaders ||
SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
-DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
+DeclRefs[ND].push_back(
+SymbolRef{SM.getFileLoc(Loc), Roles,
+ dyn_cast_or_null(ASTNode.Parent)});
// Don't continue indexing if this is a mere reference.
if (IsOnlyRef)
return true;
@@ -422,7 +424,8 @@
// Do not store references to main-file macros.
if ((static_cast(Opts.RefFilter) & Roles) && !IsMainFileOnly &&
(Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
-MacroRefs[*ID].push_back({Loc, Roles});
+// FIXME: Populate container information for macro references.
+MacroRefs[*ID].push_back({Loc, Roles, /*Container=*/nullptr});
// Collect symbols.
if (!Opts.CollectMacro)
@@ -579,24 +582,23 @@
}
return Found->second;
};
- auto CollectRef =
- [&](SymbolID ID,
- const std::pair ,
- bool Spelled = false) {
-auto FileID = SM.getFileID(LocAndRole.first);
-// FIXME: use the result to filter out references.
-shouldIndexFile(FileID);
-if (auto FileURI = GetURI(FileID)) {
- auto Range =
- getTokenRange(LocAndRole.first, SM, ASTCtx->getLangOpts());
- Ref R;
- R.Location.Start = Range.first;
- R.Location.End = Range.second;
- R.Location.FileURI = FileURI->c_str();
- R.Kind = toRefKind(LocAndRole.second, Spelled);
- Refs.insert(ID, R);
-}
- };
+ auto CollectRef = [&](SymbolID ID, const SymbolRef ,
+bool Spelled = false) {
+auto FileID = SM.getFileID(LocAndRole.Loc);
+// FIXME: use the result to filter out references.
+shouldIndexFile(FileID);
+if (auto FileURI = GetURI(FileID)) {
+ auto Range = getTokenRange(LocAndRole.Loc, SM, ASTCtx->getLangOpts());
+ Ref R;
+ R.Location.Start = Range.first;
+ R.Location.End = Range.second;
+ R.Location.FileURI = FileURI->c_str();
+ R.Kind = toRefKind(LocAndRole.Roles, Spelled);
+ if (auto RefID = getSymbolID(LocAndRole.Container))
+R.Container = *RefID;
+ Refs.insert(ID, R);
+}
+ };
// Populate Refs slab from MacroRefs.
// FIXME: All MacroRefs are marked as Spelled now, but this should be checked.
for (const auto : MacroRefs)
@@ -607,7 +609,7 @@
for (auto : DeclRefs) {
if (auto ID = getSymbolID(DeclAndRef.first)) {
for (auto : DeclAndRef.second) {
-const auto FileID = SM.getFileID(LocAndRole.first);
+const auto FileID = SM.getFileID(LocAndRole.Loc);
// FIXME: It's better to use TokenBuffer by passing spelled tokens from
// the caller of SymbolCollector.
if (!FilesToTokensCache.count(FileID))
@@ -617,7 +619,7 @@
// Check if the referenced symbol is spelled exactly the same way the
// corresponding NamedDecl is. If it is, mark this reference as spelled.
const auto *IdentifierToken =
-spelledIdentifierTouching(LocAndRole.first, Tokens);
+spelledIdentifierTouching(LocAndRole.Loc, Tokens);
DeclarationName Name = DeclAndRef.first->getDeclName();
const auto NameKind = Name.getNameKind();
bool IsTargetKind = NameKind == DeclarationName::Identifier ||
Index: clang-tools-extra/clangd/index/Serialization.cpp
===
--- clang-tools-extra/clangd/index/Serialization.cpp
