[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 4 inline comments as done.
sammccall added a comment.

PTAL



================
Comment at: clangd/ClangdServer.h:158
+  /// Retrieve locations for symbol references.
+  void references(PathRef File, Position Pos, bool includeDeclaration,
+                  Callback<std::vector<Location>> CB);
----------------
ioeric wrote:
> ioeric wrote:
> > I think the C++ API can return `SymbolOccurrence` in the callback, which would allow C++ API users to get richer information about the occurrences e.g. kind, relationship, code snippet.
> nit: s/includeDeclaration/IncludeDeclaration/
I hadn't seen this comment, I agree, but the implementation that landed just returns Locations. We can extend the C++ API but it doesn't belong in this patch.


================
Comment at: clangd/XRefs.cpp:665
+std::vector<Location> references(ParsedAST &AST, Position Pos,
+                                 bool IncludeDeclaration,
+                                 const SymbolIndex *Index) {
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Are we going to support the `IncludeDeclaration` option?
> > What should its semantics be?
> > 
> > I'm trying to figure out a better name for it, can't get what should it do by looking at the code at the moment :-)
> I think so. It is a term defined in LSP (although vscode always sets it `true`).  I think it aligns with the `clang::index::SymbolRole::Declaration`.
It's not supported in the underlying implementation today, partly because we weren't sure 
about semantics (and usefulness).

FWIW I think it should control whether the roles `Declaration` *and* `Definition` are included, because I guess it's meant to distinguish between usages that introduce a name vs usages that reference an existing name.

For now, we don't support any options and don't parse them out of the LSP.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50896