Added early exit for defaulted FunctionDecls.

[Febbe]

Summary

This prevents matching of defaulted comparison operators.
fixes llvm#53355

[Febbe]

Push, would be nice to see this in the llvm-14 release. I can also do a review for someone else as a favor.

[kbobyrev]

Mostly LG with few nits. I think it might be able to make it into the 14 release if we cherry-pick into the release branch.

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

407

nit: probably explains the situation better

411

This scope seems unnecessary.

[Febbe]

Applied feedback

[kbobyrev]

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

407

nit: Capitalization (start with T), dot at the end.

clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp

71

nit: // looks unrelated

[Febbe]

clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp

71

I used the // to break the line there (avoids // clang-format off/on)

in my opinion, it should look like this:

template <typename T> requires(T{0}) \n

[kbobyrev]

LG, thanks!

clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp

71

I think it's better spelled out explicitly (otherwise the intent is unclear) or just have the clang-format apply the formatting (since it doesn't seem very important here as it's a test).

[Febbe]

Can I still add a diff, or does this cause a revoke (apply the rest of the feedback)?
Also, is the commit added automatically to the repo, or do I / another one have to rebase it.

Sorry for those questions, this is my first contribution to llvm.

[kbobyrev]

In D118847#3307497, @Febbe wrote:

Can I still add a diff, or does this cause a revoke (apply the rest of the feedback)?
Also, is the commit added automatically to the repo, or do I / another one have to rebase it.

Sorry for those questions, this is my first contribution to llvm.

Yes, you can add changes after LGTM, it might add the warning "landed changes with unreviewed diff" but it's generally OK if the changes are the suggestions.

Someone needs to commit the patch to the monorepo. Do you have the commit access? If not, I can do that for you once you indicate you are done with the changes.

[Febbe]

Last batch of suggested changes

[Febbe]

Thank you for the review. I am done and you can commit the patch :) . I don't have the rights to commit.

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

   const auto *Fr = Result.Nodes.getNodeAs<FriendDecl>("Friend");
   assert(F && "Matcher is expected to find only FunctionDecls");
 
-  if (F->getLocation().isInvalid())
+  // Three-way comparison operator<=> is syntactic sugar and generates implicit
+  // nodes for all other operators.
+  if (F->getLocation().isInvalid() || F->isImplicit())
     return;
 
-  // Skip functions which return just 'auto'.
+  // Skip functions which return 'auto' and defaulted operators.
   const auto *AT = F->getDeclaredReturnType()->getAs<AutoType>();
-  if (AT != nullptr && !AT->isConstrained() &&
-      AT->getKeyword() == AutoTypeKeyword::Auto &&
-      !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType()))
+  if (AT != nullptr &&
+      ((!AT->isConstrained() && AT->getKeyword() == AutoTypeKeyword::Auto &&
+        !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType())) ||
+       F->isDefaulted()))
     return;
 
   // TODO: implement those

clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp

 T req2(T t) requires requires { t + t; };
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
   // CHECK-FIXES: {{^}}auto req2(T t) -> T requires requires { t + t; };{{$}}
+
+//
+// Operator c++20 defaulted comparison operators
+//
+// Requires <compare>
+
+namespace std {
+struct strong_ordering {
+  using value_type = signed char;
+  static strong_ordering const less;
+  static strong_ordering const equal;
+  static strong_ordering const equivalent;
+  static strong_ordering const greater;
+
+  constexpr strong_ordering(value_type v) : val(v) {}
+  template <typename T>
+  requires(T{0}) friend constexpr auto
+  operator==(strong_ordering v, T u) noexcept -> bool {
+    return v.val == u;
+  }
+  friend constexpr auto operator==(strong_ordering v, strong_ordering w) noexcept -> bool = default;
+
+  value_type val{};
+};
+inline constexpr strong_ordering strong_ordering::less{-1};
+inline constexpr strong_ordering strong_ordering::equal{0};
+inline constexpr strong_ordering strong_ordering::equivalent{0};
+inline constexpr strong_ordering strong_ordering::greater{1};
+
+} // namespace std
+
+struct TestDefaultOperatorA {
+  int a{};
+  int b{};
+
+  friend auto operator<=>(const TestDefaultOperatorA &, const TestDefaultOperatorA &) noexcept = default;
+};
+
+struct TestDefaultOperatorB {
+  int a{};
+  int b{};
+  friend auto operator==(const TestDefaultOperatorB &, const TestDefaultOperatorB &) noexcept -> bool = default;
+  friend bool operator<(const TestDefaultOperatorB &, const TestDefaultOperatorB &) noexcept = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
+  // CHECK-FIXES: {{^}}  friend auto operator<(const TestDefaultOperatorB &, const TestDefaultOperatorB &) noexcept -> bool = default;{{$}}
+};