[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 19 inline comments as done.
nridge added a comment.

Thank you for the reviews!



================
Comment at: clang-tools-extra/clangd/Protocol.h:1026
+  /// When not defined, it is treated as `0`.
+  llvm::Optional<int> resolve;
+
----------------
kadircet wrote:
> why not just use an int then ?
I was making the data structure correspond closely to the protocol, where the field is optional. But we can handle this in the serialization logic, yes.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:891
+        if (Optional<TypeHierarchyItem> ParentSym =
+                getTypeHierarchy(*ParentDecl, ASTCtx, Levels - 1, Direction)) {
+          Result->parents->emplace_back(std::move(*ParentSym));
----------------
kadircet wrote:
> A problem that might occur when you add children traversal:
> do we really want to include current decl in children of parent when we have `BOTH` as direction? If so, maybe we should rather cache the results? Maybe leave a todo/fixme ?
Actually, this is a bug: if the top-level call uses `Both`, the recursive calls should still just use `Parents` and `Children` (i.e. we never want children of parents or parents of children). Thanks for spotting that!


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:925
+    CXXRD = VD->getType().getTypePtr()->getAsCXXRecordDecl();
+  } else if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(D)) {
+    // If this is a method, use the type of the class.
----------------
kadircet wrote:
> what about member fields ?
It's not clear what the desired semantics would be for a member field: get the type hierarchy of the enclosing class type, or the type hierarchy of the field's type?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:935
+    TypeHierarchyDirection ResolveDirection =
+        Direction.getValueOr(TypeHierarchyDirection::Parents);
+    return getTypeHierarchy(*CXXRD, ASTCtx, ResolveLevels, ResolveDirection);
----------------
kadircet wrote:
> Is this mentioned in proposal?
It's not specified; I left a [comment on the proposal](https://github.com/Microsoft/vscode-languageserver-node/pull/426/commits/876d7fe224e17d7c08258a6da21d11a2df9c1d0d#r252942213) asking about it.


================
Comment at: clang-tools-extra/unittests/clangd/Matchers.h:130
 
+// Implements the HasValue(m) matcher for matching an Optional whose
+// value matches matcher m.
----------------
sammccall wrote:
> nridge wrote:
> > Should I split this out into a separate patch? It's used by the tests being added for this functionality.
> This is nice! I think it should probably go in llvm/Testing/Support/..., but it's OK here.
I'll leave it here for now. We can move it in a follow-up patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370