[PATCH] D153331: [clangd][c++20]Consider rewritten binary operators in TargetFinder

[Jens Massberg via Phabricator via cfe-commits]

massberg marked 2 inline comments as done.
massberg added a comment.

Thanks for the comments, I have added an additional test to FindTargetTest. See also my other comments.



================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:4051
 
+TEST(Hover, RewrittenBinaryOperatorSpaceshipMassberg) {
+  Annotations T(R"cpp(
----------------
sammccall wrote:
> 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
Upps, sorry.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:4051
 
+TEST(Hover, RewrittenBinaryOperatorSpaceshipMassberg) {
+  Annotations T(R"cpp(
----------------
massberg wrote:
> sammccall wrote:
> > 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
> Upps, sorry.
> can you add this to HoverTest__All instead? That way we test all details of the hover card

The (Hover, All) test tests with `std=c++17` while this test tests c++20 features.
We could add an additional field with the version to the struct in the (Hover, All) test.
Or add a (Hover, All_Cpp20) test for testing C++20 (what is probably not worth at the moment with just one test requiring C++20).


================
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");
----------------
sammccall wrote:
> if we're describing this as the spaceship operator, then the type looks wrong
I have added a test checking the whole definition.
Actually the following is happening here:
The `!=` operator isn't explicitly defined, so the binary operator is rewritten to `!(Foo(1) == Foo(2)`,
i.e. we are now using the `==` operator.
However, the `==`  operator is also not explicitly defined, but there is a defaulted spaceship operator.
Thus the `==` operator is implicitly defined through the `<=>` operator.
So the type and definition here are from the implicitly defined `==` operator, while the original source of it is the `<=>` operator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153331