[clang] P2085R0: Consistent defaulted comparisons

[predator5047]

Summary

Allow defaulted comparisons to be non-inline, allow their definition
to appear outside the class that declares them (as a friend).

Retain the restriction that they be declared in the relevant class
to avoid encouraging clients to add to a class’s interface.

[predator5047]

Fixed clang-tidy warning

[predator5047]

ping

[predator5047]

ping

[rsmith]

Thanks!

I think you may be missing an implementation of this rule:

A definition of a comparison operator as defaulted that appears in a class shall be the first declaration of that function.

In particular, this is now invalid:

struct A;
bool operator==(A, A);
struct A {
  friend bool operator==(A, A) = default; // error, not first declaration
};

As a subtle detail, I'm also not sure whether we handle the following rule properly, and I'd like to see some tests:

A comparison operator function for class C that is defaulted on its first declaration and is not defined as deleted is implicitly defined when it is odr-used or needed for constant evaluation.

In particular, a comparison function that's defaulted on its first declaration is defined when it's used, but if it's defaulted on a second or subsequent declaration then it's immediately defined:

template<char C> struct Broken {
  template<typename T = void> bool operator==(Broken, Broken) { return T::result; }
};

struct A { Broken<'A'> b; };
bool operator==(A, A) = default; // ok, does not try to define the function until it's used

struct B { Broken<'B'> b; };
bool operator==(B, B);
bool operator==(B, B) = default; // error, immediately defined, resulting in instantiation of Broken<'B'>::operator==, resulting in a hard error

clang/lib/Sema/SemaDeclCXX.cpp

8368–8373

I don't think you can do the "member or friend" check this way. P2085R0 doesn't require either the defaulted declaration or the first declaration to be within the class, and it doesn't matter which class contains the first declaration, if any. For example, this is valid:

struct A;
class B {
  friend bool operator==(A, A); // first declaration not in class A
  bool operator==(B);
};
class A {
  friend bool operator==(A, A);
  B b;
};
// ok, can access both A::b and B::operator==.
bool operator==(A, A) = default; // defaulted declaration not in class A

I think there are two reasonable options here:

1. First work out the class type for which we're defaulting a comparison and then traverse the list of redeclarations of the function looking for one that's lexically within the class, or
2. First work out the class type for which we're defaulting a comparison and then check that the function is either a member of that class or is in the list of that class's friends.

Either approach seems fine to me.

8614–8615

This is not necessarily the right class. The first declaration might not lexically be in a class at all, in a case like:

struct A;
bool operator==(A, A);
struct A {
  friend bool operator==(A, A);
};
bool operator==(A, A) = default;

clang/lib/Sema/SemaTemplateInstantiate.cpp

765

Passing in a function declaration here doesn't seem appropriate; this will diagnose "in defaulted equality comparison operator for 'operator==' first required here", when we wanted to say "in defaulted equality comparison operator for 'T' first required here". You'll presumably need some mechanism to determine the type being compared (something like FD->getParamDecl(0)->getType().getNonReferenceType().getUnqualifiedType(), though maybe you can share some code with CheckExplicitlyDefaultedComparison).

clang/test/CXX/class/class.compare/class.compare.default/p6.cpp

2

The file names here indicate the paragraph within the standard that's being tested; p6 would be tests for paragraph 6 of [class.compare.default]. This test doesn't appear to have anything to do with [class.compare.default] paragraph 6; it appears to be testing paragraph 1. Please can you move these tests to p1.cpp as appropriate?

[predator5047]

Addressed Richard's comments.

Fix handling of friend functions.
Added an error dialog for when enum type are used as params.
Removed define outside of class dialog since it is no longer needed.
Moved tests to p1.cpp and added more tests.

[predator5047]

Regarding

struct A;
bool operator==(A, A);
struct A {
  friend bool operator==(A, A) = default; // error, not first declaration
};

GCC and msvc have marked P2085R0 as complete and do not implement this rule should I implement it anyway?
I don't know if clang would rather adhere to the spec or do the same as gcc and msvc.
https://godbolt.org/z/nhbTWs5of

[predator5047]

Fix clang-tidy warning

[predator5047]

Fix clang-tidy formatting

[predator5047]

ping

[aaronpuchert]

In the meantime, this has apparently been addressed via D104478.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_defaulted_comparison_template : Error<
   "comparison operator template cannot be defaulted">;
 def err_defaulted_comparison_out_of_class : Error<
   "%sub{select_defaulted_comparison_kind}0 can only be defaulted in a class "
   "definition">;
 def err_defaulted_comparison_param : Error<
   "invalid parameter type for defaulted %sub{select_defaulted_comparison_kind}0"
   "; found %1, expected %2%select{| or %4}3">;
+def err_defaulted_comparison_function_not_friend : Error<
+  "defaulted %0 is not a friend of %1">;
 def err_defaulted_comparison_param_mismatch : Error<
   "parameters for defaulted %sub{select_defaulted_comparison_kind}0 "
   "must have the same type%diff{ (found $ vs $)|}1,2">;
 def err_defaulted_comparison_non_const : Error<
   "defaulted member %sub{select_defaulted_comparison_kind}0 must be "
   "const-qualified">;
 def err_defaulted_comparison_return_type_not_bool : Error<
   "return type for defaulted %sub{select_defaulted_comparison_kind}0 "

clang/lib/Sema/SemaDeclCXX.cpp

if (Op == OO_Spaceship) {
     Lookup(OO_Less);
     Lookup(OO_EqualEqual);
   }
 }
 
 bool Sema::CheckExplicitlyDefaultedComparison(Scope *S, FunctionDecl *FD,
                                               DefaultedComparisonKind DCK) {
   assert(DCK != DefaultedComparisonKind::None && "not a defaulted comparison");
+  CXXRecordDecl *RD =
+      dyn_cast<CXXRecordDecl>(FD->getCanonicalDecl()->getLexicalDeclContext());
 
-  CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(FD->getLexicalDeclContext());
-  assert(RD && "defaulted comparison is not defaulted in a class");
+  if (!RD) {
+    RD = dyn_cast<CXXRecordDecl>(FD->getLexicalDeclContext());
+  }
+
+  // If the lexical decl context is null then the function is not a friend of a
+  // record
+  if (!RD) {
+    Diag(FD->getLocation(), diag::err_defaulted_comparison_function_not_friend)
+        << FD << FD->getParamDecl(0)->getType();
+
+    return true;
+  }
 
   // Perform any unqualified lookups we're going to need to default this
   // function.
   if (S) {
     UnresolvedSet<32> Operators;
     lookupOperatorsForDefaultedComparison(*this, S, Operators,
                                           FD->getOverloadedOperator());
     FD->setDefaultedFunctionInfo(FunctionDecl::DefaultedFunctionInfo::Create(
if (!MD->isConst()) {
 
       // Add the 'const' to the type to recover.
       const auto *FPT = MD->getType()->castAs<FunctionProtoType>();
       FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
       EPI.TypeQuals.addConst();
       MD->setType(Context.getFunctionType(FPT->getReturnType(),
                                           FPT->getParamTypes(), EPI));
     }
-  } else {
-    // A non-member function declared in a class must be a friend.
-    assert(FD->getFriendObjectKind() && "expected a friend declaration");
   }
 
   // C++2a [class.eq]p1, [class.rel]p1:
   //   A [defaulted comparison other than <=>] shall have a declared return
   //   type bool.
   if (DCK != DefaultedComparisonKind::ThreeWay &&
       !FD->getDeclaredReturnType()->isDependentType() &&
       !Context.hasSameType(FD->getDeclaredReturnType(), Context.BoolTy)) {
void Sema::DefineDefaultedComparison(SourceLocation UseLoc, FunctionDecl *FD,
 
   SynthesizedFunctionScope Scope(*this, FD);
 
   // Add a context note for diagnostics produced after this point.
   Scope.addContextNote(UseLoc);
 
   {
     // Build and set up the function body.
-    CXXRecordDecl *RD = cast<CXXRecordDecl>(FD->getLexicalParent());
+
+    CXXRecordDecl *RD =
+        cast<CXXRecordDecl>(FD->getCanonicalDecl()->getLexicalDeclContext());
     SourceLocation BodyLoc =
         FD->getEndLoc().isValid() ? FD->getEndLoc() : FD->getLocation();
     StmtResult Body =
         DefaultedComparisonSynthesizer(*this, RD, FD, DCK, BodyLoc).build();
     if (Body.isInvalid()) {
       FD->setInvalidDecl();
       return;
     }
if (!DefKind &&
       (!FD->isDependentContext() ||
        (!isa<CXXConstructorDecl>(FD) &&
         FD->getDeclName().getCXXOverloadedOperator() != OO_Equal))) {
     Diag(DefaultLoc, diag::err_default_special_members)
         << getLangOpts().CPlusPlus20;
     return;
   }
 
-  if (DefKind.isComparison() &&
-      !isa<CXXRecordDecl>(FD->getLexicalDeclContext())) {
-    Diag(FD->getLocation(), diag::err_defaulted_comparison_out_of_class)
-        << (int)DefKind.asComparison();
-    return;
-  }
 
   // Issue compatibility warning. We already warned if the operator is
   // 'operator<=>' when parsing the '<=>' token.
   if (DefKind.isComparison() &&
       DefKind.asComparison() != DefaultedComparisonKind::ThreeWay) {
     Diag(DefaultLoc, getLangOpts().CPlusPlus20
                          ? diag::warn_cxx17_compat_defaulted_comparison
                          : diag::ext_defaulted_comparison);
   }
 
   FD->setDefaulted();
   FD->setExplicitlyDefaulted();
 
   // Defer checking functions that are defaulted in a dependent context.
   if (FD->isDependentContext())
     return;
 
   // Unset that we will have a body for this function. We might not,
   // if it turns out to be trivial, and we don't need this marking now
   // that we've marked it as defaulted.
   FD->setWillHaveBody(false);
 
   // If this definition appears within the record, do the checking when
-  // the record is complete. This is always the case for a defaulted
-  // comparison.
-  if (DefKind.isComparison())
+  // the record is complete. If it's defined outside the record define
+  // it now.
+  if (DefKind.isComparison()) {
+    if (!isa<CXXRecordDecl>(FD->getLexicalDeclContext()) &&
+        !CheckExplicitlyDefaultedComparison(getCurScope(), FD,
+                                            DefKind.asComparison())) {
+
+      DefineDefaultedFunction(*this, FD, DefaultLoc);
+    }
     return;
+  }
   auto *MD = cast<CXXMethodDecl>(FD);
-
   const FunctionDecl *Primary = FD;
   if (const FunctionDecl *Pattern = FD->getTemplateInstantiationPattern())
     // Ask the template instantiation pattern that actually had the
     // '= default' on it.
     Primary = Pattern;
 
   // If the method was defaulted on its first declaration, we will have
   // already performed the checking in CheckCompletedCXXClass. Such a

clang/lib/Sema/SemaTemplateInstantiate.cpp

case CodeSynthesisContext::DefiningSynthesizedFunction: {
           FD ? getDefaultedFunctionKind(FD) : DefaultedFunctionKind();
       if (DFK.isSpecialMember()) {
         auto *MD = cast<CXXMethodDecl>(FD);
         Diags.Report(Active->PointOfInstantiation,
                      diag::note_member_synthesized_at)
             << MD->isExplicitlyDefaulted() << DFK.asSpecialMember()
             << Context.getTagDeclType(MD->getParent());
       } else if (DFK.isComparison()) {
+        auto RD = dyn_cast<CXXRecordDecl>(
+            FD->getCanonicalDecl()->getLexicalDeclContext());
+        if (RD) {
           Diags.Report(Active->PointOfInstantiation,
                        diag::note_comparison_synthesized_at)
-            << (int)DFK.asComparison()
-            << Context.getTagDeclType(
-                   cast<CXXRecordDecl>(FD->getLexicalDeclContext()));
+              << (int)DFK.asComparison() << Context.getTagDeclType(RD);
+        } else {
+          Diags.Report(Active->PointOfInstantiation,
+                       diag::note_comparison_synthesized_at)
+              << (int)DFK.asComparison() << FD;
+        }
       }
       break;
     }
 
     case CodeSynthesisContext::RewritingOperatorAsSpaceship:
       Diags.Report(Active->Entity->getLocation(),
                    diag::note_rewriting_operator_as_spaceship);
       break;

clang/test/CXX/class/class.compare/class.compare.default/p1.cpp

 // RUN: %clang_cc1 -std=c++2a -verify %s
 
 struct B {};
-bool operator==(const B&, const B&) = default; // expected-error {{equality comparison operator can only be defaulted in a class definition}} expected-note {{candidate}}
-bool operator<=>(const B&, const B&) = default; // expected-error {{three-way comparison operator can only be defaulted in a class definition}}
 
 template<typename T = void>
   bool operator<(const B&, const B&) = default; // expected-error {{comparison operator template cannot be defaulted}}
 
 struct A {
   friend bool operator==(const A&, const A&) = default;
   friend bool operator!=(const A&, const B&) = default; // expected-error {{invalid parameter type for defaulted equality comparison operator; found 'const B &', expected 'A' or 'const A &'}}
   friend bool operator!=(const B&, const B&) = default; // expected-error {{invalid parameter type for defaulted equality comparison}}
struct A {
   friend bool operator>(A, const A&) = default; // expected-error {{must have the same type}} expected-note {{would be the best match}}
 
   template<typename T = void>
     friend bool operator==(const A&, const A&) = default; // expected-error {{comparison operator template cannot be defaulted}}
   template<typename T = void>
     bool operator==(const A&) const = default; // expected-error {{comparison operator template cannot be defaulted}}
 };
 
-// FIXME: The wording is not clear as to whether these are valid, but the
-// intention is that they are not.
-bool operator<(const A&, const A&) = default; // expected-error {{relational comparison operator can only be defaulted in a class definition}}
-bool A::operator<(const A&) const = default; // expected-error {{can only be defaulted in a class definition}}
-
 template<typename T> struct Dependent {
   using U = typename T::type;
   bool operator==(U) const = default; // expected-error {{found 'Dependent<Bad>::U'}}
   friend bool operator==(U, U) = default; // expected-error {{found 'Dependent<Bad>::U'}}
 };
 
 struct Good { using type = const Dependent<Good>&; };
 template struct Dependent<Good>;

clang/test/CXX/class/class.compare/class.compare.default/p6.cpp

+// RUN: %clang_cc1 -std=c++2a -verify %s
+
+// Test defaulted ref params
+struct A {
+  bool operator==(const A &) const;
+  friend bool operator!=(const A &, const A &);
+};
+
+bool A::operator==(const A &) const = default;
+bool operator!=(const A &, const A &) = default;
+
+// Test defaulted value params
+
+struct B {
+  friend bool operator==(B, B);
+  bool operator==(B) const;
+};
+bool operator==(B, B) = default;
+
+bool B::operator==(B) const = default; // expected-error {{invalid parameter type for defaulted equality comparison operator; found 'B', expected 'const B &'}}
+
+// Test for friend of C but not friend of A
+struct C {
+  friend bool operator==(const A &, const A &);
+};
+bool operator==(const A &, const A &) = default; //  expected-error {{invalid parameter type for defaulted equality comparison operator; found 'const A &', expected 'C' or 'const C &'}}
+
+struct D {
+  struct E {
+    bool operator==(const E &) const = default;
+    friend bool operator!=(const E &, const E &);
+  };
+};
+
+bool operator!=(const D::E &, const D::E &) = default;
+
+template <class T>
+struct F {
+  T __Ignore;
+  bool operator==(const F &) const = default;
+  friend bool operator!=(const F &, const F &);
+};
+
+const bool R0 = F<int>{} == F<int>{};
+
+template <class T>
+bool operator!=(const F<T> &, const F<T> &) = default; // expected-error {{comparison operator template cannot be defaulted}}
+
+struct G {
+};
+
+bool G::operator==(const G &) const = default;   // expected-error {{out-of-line definition of 'operator==' does not match any declaration in 'G'}}
+bool operator!=(const G &, const G &) = default; // expected-error  {{defaulted 'operator!=' is not a friend of 'const G &'}}
 No newline at end of file

clang/www/cxx_status.html

</tr>
         <td><a href="https://wg21.link/p1959r0">P1959R0</a></td>
       </tr>
       <tr> <!-- from Prague -->
         <td><a href="https://wg21.link/p2002r1">P2002R1</a></td>
         <td class="partial" align="center">Partial</td>
       </tr>
       <tr>
         <td><a href="https://wg21.link/p2085r0">P2085R0</a></td>
-        <td class="none" align="center">No</td>
+        <td class="none" align="center">Clang 13</td>
       </tr>
     <tr>
       <td>Access checking on specializations</td>
       <td><a href="https://wg21.link/p0692r1">P0692R1</a></td>
       <td class="partial" align="center">Partial</td>
     </tr>
     <tr>
       <td>Default constructible and assignable stateless lambdas</td>