[PATCH] D59756: [clangd] Support dependent bases in type hierarchy

[Sam McCall via Phabricator via cfe-commits]

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

LG, though I think your patch might be based on top of some uncommitted changes.

I noticed template names rather than type names are shown (not new in this patch) - we should fix that I think.



================
Comment at: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp:384
 
 TEST(TypeHierarchy, RecursiveHierarchy1) {
   Annotations Source(R"cpp(
----------------
can we give this a more descriptive name like RecursiveHierarchyUnbounded?


================
Comment at: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp:405
+      *Result,
+      AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+            Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),
----------------
Sorry, I realize this isn't related to this patch, but I didn't see the final form of the previous one.

This should be `WithName("S<0>"), ... Parents(AllOf(WithName("S<1>")), ...)`. S is the name of the template, not the name of the type.

Can you add a fixme?


================
Comment at: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp:410
 
 TEST(TypeHierarchy, RecursiveHierarchy2) {
   Annotations Source(R"cpp(
----------------
RecursiveHierarchyBounded


================
Comment at: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp:447
   struct Foo {
     S^<N> s;
   };
----------------
this can be merged with the previous test case, using two named points.


================
Comment at: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp:463
+      AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+            Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+                          SelectionRangeIs(Source.range("SDef")), Parents()))));
----------------
(in this case the printed name for the parent might end up looking odd - printing the spelled `S<N - 1>` is probably ideal but I don't really mind what we end up with, this is an edge case. Not for this patch, in any case)


================
Comment at: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp:609
 
-// Disabled for the same reason as TypeParents.DependentBase.
-TEST(DISABLED_Subtypes, DependentBase) {
+TEST(Subtypes, DependentBase) {
   Annotations Source(R"cpp(
----------------
this patch doesn't seem to be against HEAD


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59756