This is an archive of the discontinued LLVM Phabricator instance.

[clang] P2085R0: Consistent defaulted comparisons
Needs ReviewPublic

Authored by predator5047 on Jun 8 2021, 2:39 PM.

Details

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.

Diff Detail

Event Timeline

predator5047 requested review of this revision.Jun 8 2021, 2:39 PM
predator5047 created this revision.
predator5047 edited the summary of this revision. (Show Details)Jun 8 2021, 2:45 PM

Fixed clang-tidy warning

predator5047 set the repository for this revision to rG LLVM Github Monorepo.

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?

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 marked 3 inline comments as done.Jul 10 2021, 1:14 PM

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

Fix clang-tidy warning

Fix clang-tidy formatting

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

Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2023, 4:11 PM