[PATCH] D91626: [clangd] Implement textDocument/implementation.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

thanks, just reviewed at the xrefs code.  I think we can narrow the scope of the patch by splitting it into two patches

- find-implementation in xrefs.cpp
- LSP & clangd server layer (needs a smoke lit test)



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1147
+  for (const NamedDecl *ND : getDeclAtPosition(AST, *CurLoc, Relations))
+    if (llvm::isa<CXXMethodDecl>(ND))
+      Req.Subjects.insert(getSymbolID(ND));
----------------
I think we should restrict to `virtual`  method only.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1153
+  Index->relations(Req, [&](const SymbolID &Subject, const Symbol &Object) {
+    if (auto Loc = symbolToLocation(Object, *MainFilePath))
+      Results.References.push_back(*Loc);
----------------
`symbolToLocation` will prefer definition location over declaration location.

there are a few options:

1) just return declaration location
2) just return definition location
3) return both

in the find-implementaiton case, I think the declaration location is more? interesting than the definition location, I would slightly prefer 1), what do you think?


================
Comment at: clang-tools-extra/clangd/XRefs.h:87
+/// Returns implementations of the virtual function at a specified \p Pos.
+ReferencesResult findImplementations(ParsedAST &AST, Position Pos,
+                                     const SymbolIndex *Index);
----------------
not sure ReferenceResult is the best fit -- we never set the `HasMore` field. 

I'd use `std::vector<LocatedSymbol>`, this returns richer information, and the client (LSPServer can choose which location they want to return).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91626