[PATCH] D58880: [clangd] Type hierarchy subtypes

[Kadir Cetinkaya via Phabricator via cfe-commits]

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