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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
