Page MenuHomePhabricator

adds equality for spaceship types for themselves
ClosedPublic

Authored by cjdb on Jun 14 2020, 10:37 PM.

Details

Summary
  • Adds operator==(partial_ordering, partial_ordering)
  • Adds operator==(weak_ordering, weak_ordering)
  • Adds operator==(strong_ordering, strong_ordering)

Diff Detail

Event Timeline

cjdb created this revision.Jun 14 2020, 10:37 PM
cjdb updated this revision to Diff 270663.Jun 14 2020, 10:38 PM
miscco accepted this revision.Jun 14 2020, 11:38 PM

This looks obviously correct.

Thanks for working on this. I have one small nit regarding whether we need all combinations for the test but feel free to ignore it. More tests are generally better that fewer.

NOTE: I am not a maintainer so you should hold on merging this until a maintainer greenlights it. That said this seem close to the trivially correct clause
libcxx/include/compare
567

These are not part of the standard and should be handled by the rewriting of the equality operator. But definitely not part of your PR. Just raising awareness

libcxx/test/std/language.support/cmp/cmp.weakord/weakord.pass.cpp
164

I am wondering if we really need B != A if we already checked A != B on the library side.

Can we trust the compiler to have its own valid tests?

This revision is now accepted and ready to land.Jun 14 2020, 11:38 PM
cjdb marked an inline comment as done.Jun 15 2020, 8:57 AM
cjdb added a subscriber: CaseyCarter.

Thanks @miscco! Hoping this lands very soon.

NOTE: I am not a maintainer so you should hold on merging this until a maintainer greenlights it. That said this seem close to the trivially correct clause

That won't be a problem, as I don't have write access. I'll need someone to merge it on my behalf please.

libcxx/test/std/language.support/cmp/cmp.weakord/weakord.pass.cpp
164

@CaseyCarter mentioned in my same_as patch that a lot of the MSVC STL type-traits tests tend to do the testing for the compiler, which is my motivation here. Also, since I expect these names to be ultimately user-defined, I wanted to be 100% sure that they're distinct.

I have no issues deleting them if you've got strong feelings :)

cjdb added a reviewer: Restricted Project.Jun 15 2020, 9:14 AM
EricWF accepted this revision.Jun 15 2020, 11:43 AM
EricWF added inline comments.
libcxx/test/std/language.support/cmp/cmp.weakord/weakord.pass.cpp
164

I like tests. Let's keep them.

cjdb marked 2 inline comments as done.Jun 15 2020, 12:12 PM

Thanks @EricWF. Assuming your approval is synonymous with LGTM, is there anything actionable on my end before someone with write access commits it?

Is there nothing in cxx2a_status.html that needs updating? We should also update the synopsis in <compare>.

What Author Name <email> do you want this committed as?

cjdb updated this revision to Diff 270918.Jun 15 2020, 5:02 PM

Is there nothing in cxx2a_status.html that needs updating?

Done.

We should also update the synopsis in <compare>.

Done, possibly a bit overkill.

What Author Name <email> do you want this committed as?

Christopher Di Bella <cjdb@google.com> please.

cjdb added a comment.Jun 17 2020, 12:08 PM

@ldionne is there anything that remains to be done on my end, or do I just need to sit tight for now?

ldionne accepted this revision.Jun 18 2020, 7:16 AM
This revision was automatically updated to reflect the committed changes.