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