[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 a comment.

In D137894#3940600 <https://reviews.llvm.org/D137894#3940600>, @sammccall wrote:

> Since this is a protocol extension, you might want to add an end-to-end test: variant of `clang-tools-extra/clangd/test/xrefs.test` (`xrefs-container.test`? to avoid complicating all the existing tests).

I'll look into adding this, but I'm not familiar with with these kind of tests yet, so I'll might need some time to figure it out - I already uploaded an updated version of the patch anyway to get some more feedback in the meantime.

> And one this lands, one of us should update https://github.com/llvm/clangd-www/blob/main/extensions.md

Yeah I could do that, I think I can even commit directly to that repo? Or should I open a review on github/phabricator for this anyway?



================
Comment at: clang-tools-extra/clangd/Protocol.h:244
+
+  friend bool operator!=(const ReferenceLocation &LHS,
+                         const ReferenceLocation &RHS) {
----------------
sammccall wrote:
> are these operators actually needed? especially `operator<` we usually get away without
Correct, they are indeed not needed as of now.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1307
+llvm::Optional<std::string>
+getContextStringForMainFileRef(const DeclContext *DeclCtx) {
+  for (auto *Ctx = DeclCtx; Ctx; Ctx = Ctx->getParent()) {
----------------
sammccall wrote:
> a few things to consider here:
>  - for e.g. lambdas, do you want `foo::bar::(anonymous class)::operator()`, or `foo::bar`, or something else?
>  - there are a few other function-like decls, see Decl::isFunctionOrMethod().
>  - only functions? suppose you have `struct S { T member; };` and you're looking up xrefs for T. Don't we want to return `S` as the context/container name
>  - in general, I think want to get the same container for the index vs AST path here. This suggests we should be sharing `getRefContainer` from `SymbolCollector.cpp` rather than implementing something ifferent
Changed this to use `SymbolCollector::getRefContainer`, this should address most of the things you mentioned. Now AST and Index results should be consistent here


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1310
+    if (const auto *FD = llvm::dyn_cast<FunctionDecl>(Ctx))
+      return FD->getQualifiedNameAsString();
+    if (const auto *RD = llvm::dyn_cast<CXXRecordDecl>(Ctx))
----------------
sammccall wrote:
> again for stringifying, I think we want the index/non-index cases to be the same, which suggests the logic to generate Scope+Name (i.e. clangd::printQualifiedName)+Signature and then concatenating them.
> 
> Generating the signature is a bit complicated, so maybe leave it out (from both AST + index codepath) from initial patch?
There's `CodeCompletionString::getSignature`, we could use that one probably?

But anyway, the `Signature` field for index container results is actually empty right now for some reason (not sure if this a bug or a feature), so I removed Signature from index container results as well for now. Let's look into this in a follow-up patch.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1458
+          assert(Ref != RefIndexForContainer.end());
+          // Name is not useful here, because it's just gonna be the name of the
+          // function the cursor is on. Scope is interesting though, since this
----------------
sammccall wrote:
> To me, this doesn't seem like a sufficient reason to have irregular behavior for different refs.
Ok, changed it to Scope + Name here as well. If users complain about this, we can still change it later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137894