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.
--
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.
* 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.
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.
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 <=>.
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.
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.
--
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.
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.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.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:--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.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95TuYqfK%3D6fs3K8UvU1Dvif5fyFcpTHPvUO06wzwh9Xd5w%40mail.gmail.com.
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.PKOn 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?
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.
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.
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.
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;orfriend 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.)
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.
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...
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LLRVxUr5eM3Dd2WAAzE9q578O6piaXj5-mhULF5Bv%2By6g%40mail.gmail.com.
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.
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 defineconstexpr 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?
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 defineconstexpr 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);
}
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 defineconstexpr 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);
}
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 defineconstexpr 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.
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 defineconstexpr 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/60882In 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
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LLb10c1k4FeDn2K429ZVg8PgRuZSUS-YROg7G3W05LK%2Bg%40mail.gmail.com.
For a typeT
whose values can be compared for equality, define a non-memberoperator==
and document when two values of typeT
are considered equal. If there is a single obvious notion of when a valuet1
of typeT
is less than another such valuet2
then you may also defineoperator<=>
, which should be consistent withoperator==
. Prefer not to overload the other comparison and ordering operators.
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.
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?
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?
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.