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