nridge added inline comments.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1426
+ LookupRequest ContainerLookup;
+ llvm::DenseMap<SymbolID, size_t> RefIndexForContainer;
Index->relations(OverriddenBy, [&](const SymbolID &Subject,
----------------
Unlike the QueryIndex case, I think multiple references having the same
container does not apply here (different overrides will be contained in
different classes), but it's worth adding a comment to clarify that.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1484
+ LookupRequest ContainerLookup;
+ llvm::DenseMap<SymbolID, size_t> RefIndexForContainer;
Results.HasMore |= Index->refs(Req, [&](const Ref &R) {
----------------
We can have multiple references with the same container (e.g. multiple
references in the same function), so I think we need `DenseMap<SymbolID,
std::vector<size_t>>` here.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1501
}
+ SymbolID Container = R.Container;
+ ContainerLookup.IDs.insert(Container);
----------------
For good measure, perhaps condition the container-related logic here on
`AddContext`?
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2188
};
- void test(Derived* D) {
+ void test(Derived* D, Base* B) {
D->func(); // No references to the overrides.
----------------
Did you mean to add a call to `B->func()` as well here?
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2238
for (const auto &R : Code.ranges())
ExpectedLocations.push_back(rangeIs(R));
EXPECT_THAT(findReferences(AST, Code.point(), 0).References,
----------------
Did you mean to test the payload here?
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2310
+ int $decl[[foo]]() { return 42; }
+ void bar() { $bar(bar)[[foo]](); }
+ struct S { void bar() { $S(S::bar)[[foo]](); } };
----------------
nit: the payloads aren't actually used here
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2357
+ EXPECT_THAT(findReferences(AST, Main.point(), 0, /*Index=*/nullptr,
+ /*AddContext*/ true)
+ .References,
----------------
nit: we're not actually using the context
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2376
+ auto LimitRefs = findReferences(AST, Main.point(), /*Limit*/ 1,
+ IndexedTU.index().get(), /*AddContext*/
true);
EXPECT_EQ(1u, LimitRefs.References.size());
----------------
likewise
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137894/new/
https://reviews.llvm.org/D137894
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
