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

[Tom Praschan via Phabricator via cfe-commits]

tom-anders added inline comments.


================
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) {
----------------
nridge wrote:
> 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.
Good catch! Turns out this fixes the FIXME I added in FindReferences.RefsToBaseMethod


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1501
       }
+      SymbolID Container = R.Container;
+      ContainerLookup.IDs.insert(Container);
----------------
nridge wrote:
> For good measure, perhaps condition the container-related logic here on `AddContext`?
Good point, otherwise we're just wasting cycles and memory here if `AddContext` is false anyway


================
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,
----------------
nridge wrote:
> Did you mean to test the payload here?
Hmm I think I instead missed that this test does not use `checkFindRefs`. Removed the payload here, this path is already covered by other tests anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137894