This is an archive of the discontinued LLVM Phabricator instance.

[libc++][spaceship] Implement `operator<=>` for `multiset` and `set`
ClosedPublic

Authored by H-G-Hristov on Apr 15 2023, 12:32 AM.

Details

Summary

Implements parts of P1614R2

Implemented operator<=> for multiset and set

Diff Detail

Event Timeline

H-G-Hristov created this revision.Apr 15 2023, 12:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2023, 12:32 AM
Herald added a subscriber: yaxunl. · View Herald Transcript
H-G-Hristov published this revision for review.EditedMay 5 2023, 12:51 PM

I assume this passed the CI checks, so I'd like to put it for review/discussion as is.

NOTE: Unlike any other container (including std::multisite) "unorderable" values are not added to std::set. I couldn't find an explanation for that, so I hope for a feedback. I think libstdc++'s std::set behaves in the same way.
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2023, 12:51 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks for working on this. Some comments.

libcxx/include/set
1044
libcxx/test/std/containers/associative/multiset/multiset.nonmember/compare.three_way.pass.cpp
25

Does this also test containers where Compare != std::less ?

libcxx/test/support/test_container_comparisons.h
233

Does this work with a set?

@Mordante Thank you for the comments. Sorry about answering your question with questions though.

libcxx/include/set
1044

@Mordante Could you please elaborate why I need to remove inline? I am asking because operator<=> for map, list, forward_list, deque are also inline as well as the remaining operators?
If I need to remove inline here, do I need to also update the other containers for consistency?

libcxx/test/std/containers/associative/multiset/multiset.nonmember/compare.three_way.pass.cpp
25

No. I didn't consider the other cases. What other cases would you like me to add?
For consistency, I will also need to update the tests for map in that case.

libcxx/test/support/test_container_comparisons.h
233

To reduce code duplication I selected the values to work for set and multiset. I did the same for map and multimap before.
Or am I missing something?

Mordante added inline comments.May 13 2023, 3:30 AM
libcxx/include/set
1044

You can keep it if you want, but templates are implicitly inline. Sometimes we forget to point it out in reviews.

libcxx/test/std/containers/associative/multiset/multiset.nonmember/compare.three_way.pass.cpp
25

For the generic comparison it would be good to make sure the Compare does not matter. We probably should also test that maps with different Compare or Alloc can't be compared.

  • Addressed comments

Addressed comments

H-G-Hristov marked 5 inline comments as done.May 14 2023, 5:14 AM

@Mordante Thank you for the review.

libcxx/test/std/containers/associative/multiset/multiset.nonmember/compare.three_way.pass.cpp
25

I think I need a start To-Do list. Thank you for suggesting!

Mordante accepted this revision.May 22 2023, 10:26 AM

LGTM modulo one nit.

libcxx/test/support/test_comparisons.h
258–259

Unlike constexpr functions constexpr variable are not inline.

265

I actually feel std::numeric_limits<int>::min() is more readable. Especially since the name kind of indicates it's not a plain std::numeric_limits<int>::min(). So I really like this part reverted and use the numeric_limits directly.

This revision is now accepted and ready to land.May 22 2023, 10:26 AM
H-G-Hristov marked an inline comment as done.
  • Addressed comments
H-G-Hristov marked 2 inline comments as done.May 22 2023, 1:37 PM

@Mordante Thank you for the review. I reverted the change as you recommended.

H-G-Hristov added inline comments.May 22 2023, 1:43 PM
libcxx/test/support/test_comparisons.h
265

Removed both variables from above.

  • Addressed comments

Updated // expected-error tests in *.verify.cpp according the review comments in D151205

@Mordante I made a slight adjustments to the tests after your approval, if you don't mind I'll wait for a few days and I'll land it.

Mordante accepted this revision.May 27 2023, 8:54 AM
Mordante added inline comments.
libcxx/test/std/containers/associative/set/set.nonmember/compare.three_way.verify.cpp
45–46

Mainly for your information, for short lines you can use this too.
Since our CI has some issues at the moment feel free to land it as is.