kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
LGTM with a few small comments.
Thanks for implementing this awesome feature!
================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:42
+llvm::Optional<Location> symbolToLocation(const Symbol &Sym,
+ llvm::StringRef HintPath) {
----------------
let's rather return an llvm::Expected<Location> and propagate the error
messages in the error then log in the caller site
================
Comment at: clang-tools-extra/clangd/FindSymbols.h:21
class ParsedAST;
+struct Symbol;
class SymbolIndex;
----------------
let's rather `#include "Symbol.h"`
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1052
+ PathRef TUPath) {
+ // TODO: Pass in ClangdServer::WorkspaceRoot as a HintPath.
+ StringRef HintPath;
----------------
this part looks a little messed up ?
I suppose you wanted to propagate TUPath to symbolToLocation right?
================
Comment at: clang-tools-extra/clangd/XRefs.h:139
getTypeHierarchy(ParsedAST &AST, Position Pos, int Resolve,
- TypeHierarchyDirection Direction);
+ TypeHierarchyDirection Direction, PathRef TUPath = PathRef{},
+ const SymbolIndex *Index = nullptr);
----------------
I believe TUPath is not used if Index is nullptr, maybe replace those two?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58880/new/
https://reviews.llvm.org/D58880
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
