[PATCH] D96751: [clangd] Populate detail field in document symbols

[Sam McCall via Phabricator via cfe-commits]

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

Nice! This looks good to land as-is, I just have some suggestions where we may want to mark behavior to revisit later, and some places where we could trim the tests a bit.

Do you have commit access, or want me to land this for you? (I'd need an address for the commit)



================
Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:384
+                     AllOf(WithName("Foo"), WithKind(SymbolKind::Constructor),
+                           WithDetail("void ()"), Children()),
+                     AllOf(WithName("Foo"), WithKind(SymbolKind::Constructor),
----------------
the void return type isn't really meaningful

may want a FIXME here that `()` or `constructor()` would probably be better

(no need to fix though)


================
Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:392
+                     AllOf(WithName("~Foo"), WithKind(SymbolKind::Constructor),
+                           WithDetail("void ()"), Children()),
+                     AllOf(WithName("Nested"), WithKind(SymbolKind::Class),
----------------
for destructors I think we should probably suppress detail entirely - signature is always the same and it's clear from the name it's a destructor.


================
Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:490
+      ElementsAre(
+          AllOf(WithName("foo"), WithDetail("void ()")),
+          AllOf(WithName("Foo"), WithDetail("class")),
----------------
up to you, but we don't need to add WithDetail() assertions everywhere, probably just enough to ensure we cover the interesting cases.

We could leave out these ones, exportContext, noLocals etc that are really testing other things.


================
Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:588
           AllOf(WithName("Tmpl<int>"), WithKind(SymbolKind::Struct),
-                Children(WithName("y"))),
+                WithDetail("struct"),
+                Children(AllOf(WithName("y"), WithDetail("int")))),
----------------
feels like this could benefit from being "specialization struct" or so, "specialization int" below etc.

Again, this is a possible FIXME comment, not something that needs to be done now.


================
Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:630
       ElementsAreArray<::testing::Matcher<DocumentSymbol>>(
-          {AllOf(WithName("ans1"),
-                 Children(AllOf(WithName("ai1"), Children()),
-                          AllOf(WithName("ans2"), Children(WithName("ai2"))))),
-           AllOf(WithName("(anonymous namespace)"), Children(WithName("test"))),
-           AllOf(WithName("na"),
-                 Children(AllOf(WithName("nb"), Children(WithName("Foo"))))),
-           AllOf(WithName("na"),
-                 Children(AllOf(WithName("nb"), Children(WithName("Bar")))))}));
+          {AllOf(WithName("ans1"), WithDetail(""),
+                 Children(AllOf(WithName("ai1"), WithDetail("int"), Children()),
----------------
(all these WithDetail("")s seem pretty redundant)


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

https://reviews.llvm.org/D96751