[PATCH] D137894: [clangd] Add extension for adding context (enclosing function or class) in references results

[Nathan Ridge via Phabricator via cfe-commits]

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