[PATCH] D64308: [clangd] Implement typeHierarchy/resolve for subtypes

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks, I forgot we hadn't hooked this part up!



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1222
+  if (Direction == TypeHierarchyDirection::Parents || ResolveLevels == 0)
+    return llvm::None;
+
----------------
I think this should just return Item.
We're supposed to return null if the item is no longer valid, but AFAICT not if it's valid but already fully resolved.

Since we don't actually detect invalidity (e.g. when the location is no longer valid), I'd suggest just making all the functions here return TypeHierarchyItem rather than Optional (or take a mutable reference for this one, which is synchronous)


================
Comment at: clang-tools-extra/clangd/XRefs.h:144
+llvm::Optional<TypeHierarchyItem>
+resolveTypeHierarchy(TypeHierarchyItem Item, int ResolveLevels,
+                     TypeHierarchyDirection Direction,
----------------
const TypeHierarchyItem &Item

(or mutable ref as suggested in the other comment, and return void)


================
Comment at: clang-tools-extra/clangd/test/type-hierarchy.test:1
 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
----------------
Please add a unit test to TypeHierarchyTests.

I'm on the fence about having a lit test: it's a new LSP method, though the binding is pretty simple.
But features that are **only** covered by lit tests are too painful to maintain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64308