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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D103929:... Alexandru Octavian Buțiu via Phabricator via cfe-commits
    • [PATCH] D10... Alexandru Octavian Buțiu via Phabricator via cfe-commits
    • [PATCH] D10... Richard Smith - zygoloid via Phabricator via cfe-commits
    • [PATCH] D10... Alexandru Octavian Buțiu via Phabricator via cfe-commits
    • [PATCH] D10... Alexandru Octavian Buțiu via Phabricator via cfe-commits
    • [PATCH] D10... Alexandru Octavian Buțiu via Phabricator via cfe-commits
    • [PATCH] D10... Alexandru Octavian Buțiu via Phabricator via cfe-commits

Reply via email to