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