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

[Tom Praschan via Phabricator via cfe-commits]

tom-anders created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
tom-anders updated this revision to Diff 476438.
tom-anders added a comment.
tom-anders retitled this revision from "[clangd] Prototype for adding enclosing function in references results" to "[clangd] Add extension for adding context (enclosing function or class) in references results".
tom-anders published this revision for review.
tom-anders marked 2 inline comments as done.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Add tests, add client capability, add new ReferenceLocation type



================
Comment at: clang-tools-extra/clangd/Protocol.h:218
+  /// reference occurs
+  std::string container;
+
----------------
Location is a common vocabulary type.
If we're going to add an extension that relates to a particular method, we should define a new type that's wire-compatible if that's at all feasible. Could be a subclass of Location or just a clone.

(If this is just a shortcut taken while drafting this patch, please ignore me!)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1479
+
+    Index->lookup(ContainerLookup, [&](const Symbol &Container) {
+      auto Ref = RefIndexForContainer.find(Container.ID);
----------------
Being able to get all the containers in one ~constant-time index query is great! May still not want to have this on-by-default when the client doesn't advertise support, as it's a fixed nontrivial latency hit for remote index.

Also we should skip this query when the set of IDs is empty. That can go here or (maybe better) in the remote index client.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1479
+
+    Index->lookup(ContainerLookup, [&](const Symbol &Container) {
+      auto Ref = RefIndexForContainer.find(Container.ID);
----------------
sammccall wrote:
> Being able to get all the containers in one ~constant-time index query is great! May still not want to have this on-by-default when the client doesn't advertise support, as it's a fixed nontrivial latency hit for remote index.
> 
> Also we should skip this query when the set of IDs is empty. That can go here or (maybe better) in the remote index client.
> Being able to get all the containers in one ~constant-time index query is great! May still not want to have this on-by-default when the client doesn't advertise support, as it's a fixed nontrivial latency hit for remote index.
Makes sense, we could either add a ClientCapability for this (Looks like we already have some capabilities related to extensions, so this wouldn't be a novelty), or it could just be a command line flag. Any preferences? 

> Also we should skip this query when the set of IDs is empty. That can go here or (maybe better) in the remote index client.
Agreed, probably more foolproof to let the index do this (otherwise the next person will forget about that as well probably)


Relevant issue: https://github.com/clangd/clangd/issues/177


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137894

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  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/unittests/XRefsTests.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D137894.476438.patch
Type: text/x-patch
Size: 30257 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221118/9f32c8c9/attachment-0001.bin>