sammccall added a comment.
Thanks, this looks pretty good!
> I'm not sure if I the hover test which is added in this patch is the right
> one,
> but at least is passed with this patch and fails without it :)
This is nice to have, but you add a unittest to FindTargetTest too? That's the
most direct way to test this code.
This won't affect find-refs BTW, that would be handled in `refInStmt()` in
FindTarget.cpp
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:4051
+TEST(Hover, RewrittenBinaryOperatorSpaceshipMassberg) {
+ Annotations T(R"cpp(
----------------
no need to sign your work :-)
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:4051
+TEST(Hover, RewrittenBinaryOperatorSpaceshipMassberg) {
+ Annotations T(R"cpp(
----------------
sammccall wrote:
> no need to sign your work :-)
can you add this to HoverTest__All instead? That way we test all details of the
hover card
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:4079
+ EXPECT_EQ(HI->Type,
+ HoverInfo::PrintedType("bool (const Foo &) const noexcept"));
+ EXPECT_EQ(HI->Documentation, "Foo spaceship");
----------------
if we're describing this as the spaceship operator, then the type looks wrong
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153331/new/
https://reviews.llvm.org/D153331
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
