[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    [PATCH] D103929: [clang] P2085R0: Consistent defaulted comparisons
From:       Richard Smith - zygoloid via Phabricator via cfe-commits <cfe-commits () lists ! llvm
Date:       2021-06-30 21:04:52
Message-ID: s5NOSDzhTbqqzSuCBc1JYQ () geopod-ismtpd-2-1
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]