[PATCH] D103929: [clang] P2085R0: Consistent defaulted comparisons

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8368-8373
+  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());
+  }
----------------
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.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8614-8615
+
+    CXXRecordDecl *RD =
+        cast<CXXRecordDecl>(FD->getCanonicalDecl()->getLexicalDeclContext());
     SourceLocation BodyLoc =
----------------
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;
```


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:765
+                       diag::note_comparison_synthesized_at)
+              << (int)DFK.asComparison() << FD;
+        }
----------------
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`).


================
Comment at: clang/test/CXX/class/class.compare/class.compare.default/p6.cpp:1
+// RUN: %clang_cc1 -std=c++2a -verify %s
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103929