Re: [chromium-dev] When to define c++20 <=> operators for base/content classes

[Peter Kasting]

Here are my (personal, not team-blessed) rules of thumb around comparison operations:

* When possible, don't declare or define any comparison ops. The compiler will autogenerate them when it can. If nothing anywhere declares any comparison ops, everything will be comparable that can automatically be comparable. This breaks in three cases, covered by the following rules.

* When the compiler can't automatically define a correct comparison op because you need custom logic (e.g. non-lexicographical compares, skipping fields, etc.), explicitly write out <=> and ==. These two are enough. Either alone is not enough. If equality could be autogenerated but not inequalities, then write out <=> and explicitly default ==. (If they both could be defaulted, you're in the bullet above and you should declare neither.)

* When auto comparison would work but one or more member types isn't comparable, go to that type and fix it first using these same rules, then return. Note that this might mean revising or removing existing comparison ops to comply, in particular the pre-spaceship inequalities. IMO we should eliminate 100% of those over time.

* Finally, when you want to disable comparisons, explicitly delete <=> and ==. Probably just one is enough?, but both is clearer (I haven't looked into the rules).

I would be very careful around unusual cases like types that can have a meaningful == but not <=>. That sort of thing tends to lead to hard to diagnose bugs after someone sticks your type in a container later.

Finally I will forewarn you that a number of other knowledgeable folks will likely prefer explicitly-defaulted <=> and == in all cases. I disagree, but I lost the battle against always explicitly defaulting copy/move ops, so I'll probably lose this one too. :)

PK

On Sat, Nov 18, 2023, 1:39 PM 'Joe Mason' via Chromium-dev <chromi...@chromium.org> wrote:

Now that c++20 is allowed (yay!) I tried to replace the full set of comparison operators for one of my classes with operator <=>. But it failed because one class member is a `content::GlobalRenderFrameHostId` which has no operator<=> defined.

Should we go ahead and define operator<=> for common classes as needed in cases like this?

Joe

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAH%3DT95STz252cFVnH%2BEzU2mGYaK%2B40P6-99%3D%2B8yZshQ_yt-J4w%40mail.gmail.com.

[Danil Chapovalov]

But... comparison ops are not autogenerated when they are not explicitly declared. for equality at least operator== needs to be declared, for comparison - operator<=>

https://godbolt.org/z/6dz9caaW8

[Dana Jansens]

Yes, adding operator<=> (defaulted, ideally) would be good.

[Joe Mason]

[Peter Kasting]

Ugh, I misread the cppreference page. It says that if <=> is defaulted and == is not declared, == will be implicitly defaulted. But it does not say that if neither is declared, both will be defaulted. Somehow I thought it said that :P.

In that case I take back my earlier advice. Do what Danil's godbolt example does: explicitly write both operators, as in-class friends, =default.

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACQca%3De3R8muMEg0YjmM6AOXPuHLMT0z4nnm%3DeyNPj5Dvvn%3D7g%40mail.gmail.com.

[Joe Mason]

On Sat, Nov 18, 2023, 18:12 Peter Kasting <pkas...@chromium.org> wrote:

* When the compiler can't automatically define a correct comparison op because you need custom logic (e.g. non-lexicographical compares, skipping fields, etc.), explicitly write out <=> and ==. These two are enough. Either alone is not enough. If equality could be autogenerated but not inequalities, then write out <=> and explicitly default ==. (If they both could be defaulted, you're in the bullet above and you should declare neither.)

* When auto comparison would work but one or more member types isn't comparable, go to that type and fix it first using these same rules, then return. Note that this might mean revising or removing existing comparison ops to comply, in particular the pre-spaceship inequalities. IMO we should eliminate 100% of those over time.

My situation is a combo of these: I want my type to compare by an ID field and ignore it's other fields, so I want to write <=> and default ==. But the type of the ID is a class in base/ that currently has explicit < and == defined, but not <=>.

So I think the cleanest approach is to go in and convert the base/ class to <=>, and then use the obvious implementation "return id_ <=> other.id_". Just checking that base/ owners are ready to accept this kind of change to individual classes as needed, or if there should be a more deliberate conversion strategy.

If I'm reading this right, the alternative is to write my <=> as:

if (id_ < other.id_) return std::strict_ordering::less;

else if (id_ == other.id_) return std::strict_ordering::equal;

else return std::strict_ordering::greater;

[Dana Jansens]

On Tue, Nov 21, 2023 at 12:36 PM 'Joe Mason' via cxx <c...@chromium.org> wrote:

On Sat, Nov 18, 2023, 18:12 Peter Kasting <pkas...@chromium.org> wrote:

* When the compiler can't automatically define a correct comparison op because you need custom logic (e.g. non-lexicographical compares, skipping fields, etc.), explicitly write out <=> and ==. These two are enough. Either alone is not enough. If equality could be autogenerated but not inequalities, then write out <=> and explicitly default ==. (If they both could be defaulted, you're in the bullet above and you should declare neither.)

* When auto comparison would work but one or more member types isn't comparable, go to that type and fix it first using these same rules, then return. Note that this might mean revising or removing existing comparison ops to comply, in particular the pre-spaceship inequalities. IMO we should eliminate 100% of those over time.

My situation is a combo of these: I want my type to compare by an ID field and ignore it's other fields, so I want to write <=> and default ==. But the type of the ID is a class in base/ that currently has explicit < and == defined, but not <=>.

So I think the cleanest approach is to go in and convert the base/ class to <=>, and then use the obvious implementation "return id_ <=> other.id_". Just checking that base/ owners are ready to accept this kind of change to individual classes as needed, or if there should be a more deliberate conversion strategy.

Yes, please do this. :)

 

If I'm reading this right, the alternative is to write my <=> as:

if (id_ < other.id_) return std::strict_ordering::less;

else if (id_ == other.id_) return std::strict_ordering::equal;

else return std::strict_ordering::greater;

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95SRUDKsYmdFfxahtBPoLF12Jo%3DkQ0qM%2B%2BL-gZmc_PAO4A%40mail.gmail.com.

[Kyle Charbonneau]

My situation is a combo of these: I want my type to compare by an ID field and ignore it's other fields, so I want to write <=> and default ==. But the type of the ID is a class in base/ that currently has explicit < and == defined, but not <=>.

One minor addition, I believe your classes operator<=>() should return std::weak_ordering and not std::strong_ordering if two instances that are equivalent according to operator<=>() are distinguishable. 

std::strong_ordering

Implies substitutability: if a is equivalent to b, f(a) is also equivalent to f(b), where f denotes a function that reads only comparison-salient state that is accessible via the argument's public const members. In other words, equivalent values are indistinguishable. 

 

std::weak_ordering

Does not imply substitutability: if a is equivalent to b, f(a) may not be equivalent to f(b), where f denotes a function that reads only comparison-salient state that is accessible via the argument's public const members. In other words, equivalent values may be distinguishable.

On Tue, Nov 21, 2023 at 12:36 PM 'Joe Mason' via cxx <c...@chromium.org> wrote:

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95SRUDKsYmdFfxahtBPoLF12Jo%3DkQ0qM%2B%2BL-gZmc_PAO4A%40mail.gmail.com.

[Dana Jansens]

On Tue, Nov 21, 2023 at 3:57 PM Kyle Charbonneau <kyle...@chromium.org> wrote:

My situation is a combo of these: I want my type to compare by an ID field and ignore it's other fields, so I want to write <=> and default ==. But the type of the ID is a class in base/ that currently has explicit < and == defined, but not <=>.

One minor addition, I believe your classes operator<=>() should return std::weak_ordering and not std::strong_ordering if two instances that are equivalent according to operator<=>() are distinguishable. 

std::strong_ordering

Implies substitutability: if a is equivalent to b, f(a) is also equivalent to f(b), where f denotes a function that reads only comparison-salient state that is accessible via the argument's public const members. In other words, equivalent values are indistinguishable. 

 

std::weak_ordering

Does not imply substitutability: if a is equivalent to b, f(a) may not be equivalent to f(b), where f denotes a function that reads only comparison-salient state that is accessible via the argument's public const members. In other words, equivalent values may be distinguishable.

+1. In fact weak_ordering is sufficient for almost everything, so using it is a safe default. I think we may have been better off without strong_ordering at all but alas.

On Tue, Nov 21, 2023 at 12:36 PM 'Joe Mason' via cxx <c...@chromium.org> wrote:

On Sat, Nov 18, 2023, 18:12 Peter Kasting <pkas...@chromium.org> wrote:

* When the compiler can't automatically define a correct comparison op because you need custom logic (e.g. non-lexicographical compares, skipping fields, etc.), explicitly write out <=> and ==. These two are enough. Either alone is not enough. If equality could be autogenerated but not inequalities, then write out <=> and explicitly default ==. (If they both could be defaulted, you're in the bullet above and you should declare neither.)

* When auto comparison would work but one or more member types isn't comparable, go to that type and fix it first using these same rules, then return. Note that this might mean revising or removing existing comparison ops to comply, in particular the pre-spaceship inequalities. IMO we should eliminate 100% of those over time.

My situation is a combo of these: I want my type to compare by an ID field and ignore it's other fields, so I want to write <=> and default ==. But the type of the ID is a class in base/ that currently has explicit < and == defined, but not <=>.

So I think the cleanest approach is to go in and convert the base/ class to <=>, and then use the obvious implementation "return id_ <=> other.id_". Just checking that base/ owners are ready to accept this kind of change to individual classes as needed, or if there should be a more deliberate conversion strategy.

If I'm reading this right, the alternative is to write my <=> as:

if (id_ < other.id_) return std::strict_ordering::less;

else if (id_ == other.id_) return std::strict_ordering::equal;

else return std::strict_ordering::greater;

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95SRUDKsYmdFfxahtBPoLF12Jo%3DkQ0qM%2B%2BL-gZmc_PAO4A%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAERSRDDvQK%2B7ok1JVHjV-qkCwsz69e06BcwFSE-ofJGFo67wsQ%40mail.gmail.com.

[Joe Mason]

Huh, I had weak_ordering all wrong - I see now that the concept I was thinking of is called partial_ordering. Thanks for the additional info!

Sounds like this could use an explainer.

Speaking of which, another question: my understanding is that with operator rewriting, defining `T1::operator==(const T2& other)` now implicitly handles T2==T1 as well as T1==T2. Does that mean we should now prefer member operators over non-member friend functions?

[Peter Kasting]

The biggest source of compile failures I fixed to move to c++20 was operator reordering exposing non-symmetric operator definitions. This only causes compile failures once a caller uses things in an ambiguous way, though, so it's still possible to write time bomb APIs.

IMO non-member friends make such asymmetry harder and more obvious, and as such should still be preferred.

PK

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95TuYqfK%3D6fs3K8UvU1Dvif5fyFcpTHPvUO06wzwh9Xd5w%40mail.gmail.com.

[Dana Jansens]

On Wed, Nov 22, 2023 at 10:35 AM Peter Kasting <pkas...@chromium.org> wrote:

The biggest source of compile failures I fixed to move to c++20 was operator reordering exposing non-symmetric operator definitions. This only causes compile failures once a caller uses things in an ambiguous way, though, so it's still possible to write time bomb APIs.

IMO non-member friends make such asymmetry harder and more obvious, and as such should still be preferred.

PK

On Wed, Nov 22, 2023, 6:15 AM 'Joe Mason' via cxx <c...@chromium.org> wrote:

Huh, I had weak_ordering all wrong - I see now that the concept I was thinking of is called partial_ordering. Thanks for the additional info!

Sounds like this could use an explainer.

Speaking of which, another question: my understanding is that with operator rewriting, defining `T1::operator==(const T2& other)` now implicitly handles T2==T1 as well as T1==T2. Does that mean we should now prefer member operators over non-member friend functions?

What PK said, but to add a bit more. The symmetry property is not only for member operators, it will be done for friend functions too. Also, they also give you operator!= for free.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDUqa68EPTHLcT9d6H5Q8a%2BG6_USUX-f0V5yKogNwMkzA%40mail.gmail.com.

[Gabriel Charette]

On Tue, Nov 21, 2023 at 1:23 PM Dana Jansens <dan...@chromium.org> wrote:

On Tue, Nov 21, 2023 at 12:36 PM 'Joe Mason' via cxx <c...@chromium.org> wrote:

On Sat, Nov 18, 2023, 18:12 Peter Kasting <pkas...@chromium.org> wrote:

* When the compiler can't automatically define a correct comparison op because you need custom logic (e.g. non-lexicographical compares, skipping fields, etc.), explicitly write out <=> and ==. These two are enough. Either alone is not enough. If equality could be autogenerated but not inequalities, then write out <=> and explicitly default ==. (If they both could be defaulted, you're in the bullet above and you should declare neither.)

* When auto comparison would work but one or more member types isn't comparable, go to that type and fix it first using these same rules, then return. Note that this might mean revising or removing existing comparison ops to comply, in particular the pre-spaceship inequalities. IMO we should eliminate 100% of those over time.

My situation is a combo of these: I want my type to compare by an ID field and ignore it's other fields, so I want to write <=> and default ==. But the type of the ID is a class in base/ that currently has explicit < and == defined, but not <=>.

So I think the cleanest approach is to go in and convert the base/ class to <=>, and then use the obvious implementation "return id_ <=> other.id_". Just checking that base/ owners are ready to accept this kind of change to individual classes as needed, or if there should be a more deliberate conversion strategy.

Yes, please do this. :)

+1 :), I'm excited to finally have comparisons be = default; in the majority of cases.

 

 

If I'm reading this right, the alternative is to write my <=> as:

if (id_ < other.id_) return std::strict_ordering::less;

else if (id_ == other.id_) return std::strict_ordering::equal;

else return std::strict_ordering::greater;

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95SRUDKsYmdFfxahtBPoLF12Jo%3DkQ0qM%2B%2BL-gZmc_PAO4A%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQVCVz%2Bm-WzpvK95hV7kF8wHFoyPQFWMtAsVtoGBj7zXQ%40mail.gmail.com.

[Joe Mason]

I'll send you some patches. :-)

So to summarize my takeaways from this thread:

1. If your class has a defined ordering, and all its members are used to define the ordering, define <=> as default. (This will automatically define == as default too.)

    friend constexpr auto operator<=>(const MyClass& lhs, const MyClass& rhs) = default;

2. If your class has a defined ordering but only some of its members are involved, give explicit definitions of <=> and ==.

    friend constexpr auto operator<=>(const MyClass& lhs, const MyClass& rhs) {

      return std::tie(lhs.key_field1_, lhs.key_field2_) <=> std::tie(rhs.key_field1_, rhs.key_field2_);

    }

    friend constexpr bool operator==(const MyClass& lhs, const MyClass& rhs) {

      return lhs.key_field1_ == rhs.key_field1_ && lhs.key_field2_ == rhs.key_field2_;

    }

3. If it doesn't have a defined ordering, only define ==. Default it if all members should be checked for equality, give an explicit definition if not.

    friend constexpr bool operator==(const MyClass& lhs, const MyClass& rhs) = default;

or

    friend constexpr bool operator==(const MyClass& lhs, const MyClass& rhs) { return lhs.key_field_ == rhs.key_field_; }

4. If the ordering needs to compare members that don't themselves have <=> defined, prefer to modify the members to support <=>. If this isn't possible write <=> explicitly using the pre-C++20 comparison operators and std::weak_ordering. Only use std::strong_ordering if equal instances of the class should be considered identical, not just equivalent (eg. two base::Token objects deserialized from the same bytes).

    friend constexpr auto operator<=>(const MyClass& lhs, const MyClass& rhs) {

      if (lhs.key_field_ < rhs.key_field_) {

        return std::weak_ordering::less;

      }

      if (lhs.key_field_ == rhs.key_field_) {

        return std::weak_ordering::equivalent;

      }

      return std::weak_ordering::greater;

    }

    friend constexpr bool operator==(const MyClass& lhs, const MyClass& rhs) { return lhs.key_field_ == rhs.key_field_; }

(All of these examples use constexpr, because it doesn't hurt, but generally leave off the constexpr if it fails to compile.)

Does that about cover it?

[Dana Jansens]

On Mon, Nov 27, 2023 at 1:38 PM Joe Mason <joenot...@google.com> wrote:

I'll send you some patches. :-)

So to summarize my takeaways from this thread:

1. If your class has a defined ordering, and all its members are used to define the ordering, define <=> as default. (This will automatically define == as default too.)

    friend constexpr auto operator<=>(const MyClass& lhs, const MyClass& rhs) = default;

IMO we should write operator== as default anyway. I don't expect or want people to have to memorize more implicit stuff.

2. If your class has a defined ordering but only some of its members are involved, give explicit definitions of <=> and ==.

    friend constexpr auto operator<=>(const MyClass& lhs, const MyClass& rhs) {

      return std::tie(lhs.key_field1_, lhs.key_field2_) <=> std::tie(rhs.key_field1_, rhs.key_field2_);

    }

    friend constexpr bool operator==(const MyClass& lhs, const MyClass& rhs) {

      return lhs.key_field1_ == rhs.key_field1_ && lhs.key_field2_ == rhs.key_field2_;

    }

3. If it doesn't have a defined ordering, only define ==. Default it if all members should be checked for equality, give an explicit definition if not.

    friend constexpr bool operator==(const MyClass& lhs, const MyClass& rhs) = default;

or

    friend constexpr bool operator==(const MyClass& lhs, const MyClass& rhs) { return lhs.key_field_ == rhs.key_field_; }

4. If the ordering needs to compare members that don't themselves have <=> defined, prefer to modify the members to support <=>. If this isn't possible write <=> explicitly using the pre-C++20 comparison operators and std::weak_ordering. Only use std::strong_ordering if equal instances of the class should be considered identical, not just equivalent (eg. two base::Token objects deserialized from the same bytes).

Yeah, another way to say this is strong_ordering means that equivalently ordered objects are replaceable/interchangeable.

    friend constexpr auto operator<=>(const MyClass& lhs, const MyClass& rhs) {

I would write a non-auto return type since it's not a dependent type on what the fields' ordering type is/are.

 

      if (lhs.key_field_ < rhs.key_field_) {

        return std::weak_ordering::less;

      }

      if (lhs.key_field_ == rhs.key_field_) {

        return std::weak_ordering::equivalent;

      }

      return std::weak_ordering::greater;

    }

    friend constexpr bool operator==(const MyClass& lhs, const MyClass& rhs) { return lhs.key_field_ == rhs.key_field_; }

(All of these examples use constexpr, because it doesn't hurt, but generally leave off the constexpr if it fails to compile.)

constexpr will let the compiler inline these appropriately, which is good, although we also don't like inlining due to binary size. I do think we want to use constexpr for these.

[Gabriel Charette]

On Mon, Nov 27, 2023 at 1:44 PM Dana Jansens <dan...@chromium.org> wrote:

On Mon, Nov 27, 2023 at 1:38 PM Joe Mason <joenot...@google.com> wrote:

I'll send you some patches. :-)

So to summarize my takeaways from this thread:

1. If your class has a defined ordering, and all its members are used to define the ordering, define <=> as default. (This will automatically define == as default too.)

    friend constexpr auto operator<=>(const MyClass& lhs, const MyClass& rhs) = default;

IMO we should write operator== as default anyway. I don't expect or want people to have to memorize more implicit stuff.

I don't see why we should define operator== as default alongside a default operator<=>, seems to defeat the point of operator<=> being succinct?

e.g.

  friend constexpr auto operator<=>(const ResultMetadata&,
                                    const ResultMetadata&) = default;
  friend constexpr bool operator==(const ResultMetadata&,
                                   const ResultMetadata&) = default;

 is redundant IMO...

[Daniel Cheng]

On Mon, 27 Nov 2023 at 13:04, Gabriel Charette <g...@chromium.org> wrote:

On Mon, Nov 27, 2023 at 1:44 PM Dana Jansens <dan...@chromium.org> wrote:

On Mon, Nov 27, 2023 at 1:38 PM Joe Mason <joenot...@google.com> wrote:

I'll send you some patches. :-)

So to summarize my takeaways from this thread:

1. If your class has a defined ordering, and all its members are used to define the ordering, define <=> as default. (This will automatically define == as default too.)

    friend constexpr auto operator<=>(const MyClass& lhs, const MyClass& rhs) = default;

IMO we should write operator== as default anyway. I don't expect or want people to have to memorize more implicit stuff.

I don't see why we should define operator== as default alongside a default operator<=>, seems to defeat the point of operator<=> being succinct?

e.g.

  friend constexpr auto operator<=>(const ResultMetadata&,
                                    const ResultMetadata&) = default;
  friend constexpr bool operator==(const ResultMetadata&,
                                   const ResultMetadata&) = default;

 is redundant IMO...

Two reasons:

1. I hate remembering the rules for things the compiler implicitly generates.

2. The style guide says so: https://google.github.io/styleguide/cppguide.html#:~:text=For%20a%20type,and%20ordering%20operators.

Daniel

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LLRVxUr5eM3Dd2WAAzE9q578O6piaXj5-mhULF5Bv%2By6g%40mail.gmail.com.

[Gabriel Charette]

On Mon, Nov 27, 2023 at 4:09 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, 27 Nov 2023 at 13:04, Gabriel Charette <g...@chromium.org> wrote:

On Mon, Nov 27, 2023 at 1:44 PM Dana Jansens <dan...@chromium.org> wrote:

On Mon, Nov 27, 2023 at 1:38 PM Joe Mason <joenot...@google.com> wrote:

I'll send you some patches. :-)

So to summarize my takeaways from this thread:

1. If your class has a defined ordering, and all its members are used to define the ordering, define <=> as default. (This will automatically define == as default too.)

    friend constexpr auto operator<=>(const MyClass& lhs, const MyClass& rhs) = default;

IMO we should write operator== as default anyway. I don't expect or want people to have to memorize more implicit stuff.

I don't see why we should define operator== as default alongside a default operator<=>, seems to defeat the point of operator<=> being succinct?

e.g.

  friend constexpr auto operator<=>(const ResultMetadata&,
                                    const ResultMetadata&) = default;
  friend constexpr bool operator==(const ResultMetadata&,
                                   const ResultMetadata&) = default;

 is redundant IMO...

Two reasons:

1. I hate remembering the rules for things the compiler implicitly generates.

2. The style guide says so: https://google.github.io/styleguide/cppguide.html#:~:text=For%20a%20type,and%20ordering%20operators.

I see. I feel like defining both opens the door to their definition potentially becoming inconsistent, but the style guide prevails over my opinion :).

[Joe Mason]

The only way I can see for them to become inconsistent is if <=> is defined as default and later given a custom implementation. In that case, if == is explicitly default it will silently stay that way (bad), but if it's not defined equality comparisons will no longer compile until it's updated (good).

So to be really safe we should define

constexpr friend auto operator<=>(const CompareTest& lhs, const CompareTest& rhs) = default;
constexpr friend bool operator==(const CompareTest& lhs, const CompareTest& rhs) {
  return std::is_eq(lhs <=> rhs);
}

But that seems excessive?

[Dana Jansens]

On Mon, Nov 27, 2023 at 5:00 PM Joe Mason <joenot...@google.com> wrote:

The only way I can see for them to become inconsistent is if <=> is defined as default and later given a custom implementation. In that case, if == is explicitly default it will silently stay that way (bad), but if it's not defined equality comparisons will no longer compile until it's updated (good).

So to be really safe we should define

constexpr friend auto operator<=>(const CompareTest& lhs, const CompareTest& rhs) = default;
constexpr friend bool operator==(const CompareTest& lhs, const CompareTest& rhs) {
  return std::is_eq(lhs <=> rhs);
}

But that seems excessive?

And this is only right if operator<=> returns strong_ordering.

[Daniel Cheng]

On Mon, 27 Nov 2023 at 14:04, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Nov 27, 2023 at 5:00 PM Joe Mason <joenot...@google.com> wrote:

The only way I can see for them to become inconsistent is if <=> is defined as default and later given a custom implementation. In that case, if == is explicitly default it will silently stay that way (bad), but if it's not defined equality comparisons will no longer compile until it's updated (good).

So to be really safe we should define

constexpr friend auto operator<=>(const CompareTest& lhs, const CompareTest& rhs) = default;
constexpr friend bool operator==(const CompareTest& lhs, const CompareTest& rhs) {
  return std::is_eq(lhs <=> rhs);
}

I don't think it's generally recommended to implement == in terms of <=> though: isn't the whole idea behind == that it can short-circuit very quickly sometimes?

Daniel

[Gabriel Charette]

On Mon, Nov 27, 2023 at 5:00 PM Joe Mason <joenot...@google.com> wrote:

The only way I can see for them to become inconsistent is if <=> is defined as default and later given a custom implementation. In that case, if == is explicitly default it will silently stay that way (bad), but if it's not defined equality comparisons will no longer compile until it's updated (good).

So to be really safe we should define

constexpr friend auto operator<=>(const CompareTest& lhs, const CompareTest& rhs) = default;
constexpr friend bool operator==(const CompareTest& lhs, const CompareTest& rhs) {
  return std::is_eq(lhs <=> rhs);
}

I prefer both defaulted to this, even though it's redundant to me.

[Daniel Cheng]

On Mon, 27 Nov 2023 at 14:37, Gabriel Charette <g...@chromium.org> wrote:

On Mon, Nov 27, 2023 at 5:00 PM Joe Mason <joenot...@google.com> wrote:

The only way I can see for them to become inconsistent is if <=> is defined as default and later given a custom implementation. In that case, if == is explicitly default it will silently stay that way (bad), but if it's not defined equality comparisons will no longer compile until it's updated (good).

So to be really safe we should define

constexpr friend auto operator<=>(const CompareTest& lhs, const CompareTest& rhs) = default;
constexpr friend bool operator==(const CompareTest& lhs, const CompareTest& rhs) {
  return std::is_eq(lhs <=> rhs);
}

I prefer both defaulted to this, even though it's redundant to me.

This isn't good guidance though. Rust doesn't provide a blanket implementation for Eq based on Ord: https://github.com/rust-lang/rust/issues/60882

In earlier proposals, C++ also provided a blanket implementation of all the comparison operators based on <=>, but moved away from it due to the performance footguns: https://open-std.org/JTC1/SC22/WG21/docs/papers/2019/p1185r2.html

Daniel

[Gabriel Charette]

On Mon, Nov 27, 2023 at 5:58 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, 27 Nov 2023 at 14:37, Gabriel Charette <g...@chromium.org> wrote:

On Mon, Nov 27, 2023 at 5:00 PM Joe Mason <joenot...@google.com> wrote:

The only way I can see for them to become inconsistent is if <=> is defined as default and later given a custom implementation. In that case, if == is explicitly default it will silently stay that way (bad), but if it's not defined equality comparisons will no longer compile until it's updated (good).

So to be really safe we should define

constexpr friend auto operator<=>(const CompareTest& lhs, const CompareTest& rhs) = default;
constexpr friend bool operator==(const CompareTest& lhs, const CompareTest& rhs) {
  return std::is_eq(lhs <=> rhs);
}

I prefer both defaulted to this, even though it's redundant to me.

This isn't good guidance though. Rust doesn't provide a blanket implementation for Eq based on Ord: https://github.com/rust-lang/rust/issues/60882

In earlier proposals, C++ also provided a blanket implementation of all the comparison operators based on <=>, but moved away from it due to the performance footguns: https://open-std.org/JTC1/SC22/WG21/docs/papers/2019/p1185r2.html

Oh! Yikes! Was this adopted? Or do we need to also default operator!= as mentioned?

The current recommendation seems to hint it was adopted, but not the follow-up proposal to make operator<=> also default operator== (which is what I was incorrectly assuming was the case).

[Joe Mason]

I think it's correct for all orderings - std::is_eq is defined for strong_ordering, weak_ordering and partial_ordering, so the correct overload should be invoked for the return type of <=>.

Performance is another issue. 

[Daniel Cheng]

No, we just need operator== and operator<=>. There may be times that it's useful to define == in terms of <=>, but I don't think it's something we should steer people towards.

Daniel

[Joe Mason]

Yeah, I withdraw the suggestion - I actually read about that performance issue the other day, and then forgot all about it.

[Peter Kasting]

Operator rewriting means that defaulting == is enough and != does not need to be defaulted. Defaulting <=> does implicitly default ==, but I don't mind being explicit about that, since the defaulted == is not implemented in terms of <=>.

PK

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LLb10c1k4FeDn2K429ZVg8PgRuZSUS-YROg7G3W05LK%2Bg%40mail.gmail.com.

[Gabriel Charette]

Thanks Peter, I also just found that If operator<=> is defaulted and operator== is not declared at all, then operator== is implicitly defaulted. So declaring operator== as default is indeed redundant (in other words, the spec got it right after the modification which Daniel linked to earlier).

But our style guide says to explicitly declare operator== so let's do that nonetheless...

[Joe Mason]

I actually think the style guide is ambiguous on whether operator== must be explicit. The language is: 

For a type T whose values can be compared for equality, define a non-member operator== and document when two values of type T are considered equal. If there is a single obvious notion of when a value t1 of type T is less than another such value t2 then you may also define operator<=>, which should be consistent with operator==. Prefer not to overload the other comparison and ordering operators.

Which doesn't explicitly address the defaulted case. I could read this as talking about hand-written operator== and operator<=>, and meaning to make sure you don't define <=> without ==. Since default operator<=> results in a consistent operator== being defined, that could fit the spirit of the wording. (Note also it requires to "document when two values of type T are considered equal" - I wouldn't require that for the default operators, since they're self-documenting: two values are equal if all members are equal.)

Nonetheless after this discussion I do think we should ask for explicit operator==, because the chance of them getting out of sync is low (they're generally defined right next to each other, so easy to spot that they should be updated in sync) and it reinforces the idea that == and <=> are separate.

(Is it worth a clang plugin? Chrome-specific styleguide or Do's and Don'ts entry?)

[K. Moon]

I wouldn't read "define" narrowly here to only mean hand-written definitions; defaulted definitions are still definitions, in the C++ sense. The excluded cases are (1) not declaring operator==, and (2) declaring but not defining operator==.

The way I read this, the basic algorithm the style guide is describing is:

1. If equality makes sense for T, define operator== as a non-member function.

2. If comparison makes sense for T, define operator<=> consistent with operator==.

Note that you have to get to step (1) before you can execute step (2).

That said, if it's confusing, then an update with more explicit language is probably a good idea. The style guide is supposed to be clear, not a legal text. :-)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95Q79o64-Yj-oapkyaZ4fFRnb2KG7kYVwe9MrPU2gSD_Pw%40mail.gmail.com.

[Joe Mason]

A question just came up in code review (https://crrev.com/c/5054490):

Thoughout Chromium we avoid putting function bodies in headers except where unavoidable, or for simple accessors (see https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#inline-functions) - you'll typically see that classes with default ctor/dtor have those declared in the .cc file rather than the header.

Shouldn't these "default" operators be dealt with similarly?

kylechar gave a pretty good answer in the review:

It's painful to do this with non-member operators since you need to write the operators in ~3 places.

1. Declare the friend function inside the class (if required)
2. Declare the functions with appropriate exports in the header
3. Define = default the functions in a source file 

With member functions it's only declare in class header and then define in the source file which is a little better but not allowed anyways.

The recommendation to put constructor/destructor definition in a source file is mostly about binary size thought right? This CL has -72 byte binary size impact so it doesn't seem to be hurting in this specific case. I'd imagine that is due to many comparison operators already being define inline, being able to remove operator!=() always and sometimes replace four operators with operator<=>().

I guess we should clarify this in the Chrome style guide?

Any thoughts on this? 

[Peter Kasting]

We'd default things in headers in all cases if it had no binary size impact; putting them in .cc files isn't preferred, it's just mandated by our plugin sometimes. Someday that may change.

I don't think it matters too much what people do here, not enough to pull its weight as a style rule. Every rule reduces the memorability and impact of all the rules, so we should only add one when the answer really matters.

PK

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95T8gfK%3Dde%2B%2B5V7UTKHRhLDAcrUXbAPXN7zJDGyZfGCF4Q%40mail.gmail.com.

[Daniel Cheng]

It probably has a binary size impact. But it's already known that the implicit ctors/dtors warning is painful, and I don't plan on adding more enforcement of this; instead, I'm hoping to carve out some time to investigate compiler changes that will hopefully prevent us from having to think about these sorts of issues at all.

Daniel