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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Protocol.h:231
 
+/// Special Location-like type to be used for textDocument/references
+/// Contains an additional field for the context in which the reference occurs
----------------
This doesn't seem as clear as it could be: special how, additional to what? Maybe:

```
/// Extends Locations returned by textDocument/references with extra info.
/// This is a clangd extenison: LSP uses `Location`.
```


================
Comment at: clang-tools-extra/clangd/Protocol.h:236
+  /// reference occurs
+  llvm::Optional<std::string> context;
+
----------------
"context" is vague, and is generally used in LSP to mean "details about how a request was triggered in the client".

Unless we have a reason to keep this abstract, I'd use `containerName` as it's used elsewhere in LSP for this purpose.
(I kind of hate the name, but at least people will be familiar with it)


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


================
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()) {
----------------
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


================
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))
----------------
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?


================
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
----------------
To me, this doesn't seem like a sufficient reason to have irregular behavior for different refs.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1462
+          Results.References[Ref->getSecond()].Loc.context =
+              Container.Scope.drop_back(2).str(); // Drop trailing "::"
+        });
----------------
nit: unconditional drop_back(2) seems brittle, consume_back instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137894