This is an archive of the discontinued LLVM Phabricator instance.

[libc++][spaceship] Implement `operator<=>` for `queue`
ClosedPublic

Authored by H-G-Hristov on Mar 14 2023, 10:13 AM.

Details

Reviewers
Mordante
Group Reviewers
Restricted Project
Commits
rG172d990c0394: [libc++][spaceship] Implement `operator<=>` for `queue`
Summary

Implements parts of P1614R2 operator<=> for queue

Diff Detail

Event Timeline

H-G-Hristov created this revision.Mar 14 2023, 10:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 10:13 AM
Herald added a subscriber: yaxunl. · View Herald Transcript
H-G-Hristov requested review of this revision.Mar 14 2023, 10:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 10:13 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik added inline comments.
libcxx/test/std/containers/container.adaptors/queue/compare.three_way.pass.cpp
25

We should also test with a custom container to make sure it actually calls operator<=> and not std::lexicographical_compare_three_way(x.begin(), x.end(), y.begin(), y.end()).

Can you fix the CI and address the open review comments, I prefer to review with the CI results.

H-G-Hristov planned changes to this revision.Mar 22 2023, 2:48 PM

Thank you for the review. I still need to add tests with a custom container.

  • Rebased & tweaks
H-G-Hristov planned changes to this revision.Apr 15 2023, 12:22 AM
  • Try to fix CI
H-G-Hristov planned changes to this revision.Apr 15 2023, 4:25 AM
  • Try to fix CI
H-G-Hristov planned changes to this revision.Apr 15 2023, 11:08 AM
H-G-Hristov planned changes to this revision.May 5 2023, 12:13 PM
H-G-Hristov planned changes to this revision.May 29 2023, 10:48 PM

Rebased try to fix CI

H-G-Hristov marked an inline comment as done.Jun 1 2023, 11:06 AM
H-G-Hristov added inline comments.
libcxx/test/std/containers/container.adaptors/queue/compare.three_way.pass.cpp
25

I added a "custom container" test I hope that it is custom enough.

In general I'm happy, but I would like to see whether custom_container_iterator can be removed.

libcxx/test/support/test_container_comparisons.h
20

Would it be possible to use nasty_vector from "nasty_containers.h" instead?

You will need to add

#if TEST_STD_VER >= 20
template <class T>
auto operator<=>(const nasty_vector<T>& x, const nasty_vector<T>& y) { return x.v_ <=> y.v_; }
#endif

but that seems easier than to add a new class.

H-G-Hristov marked an inline comment as done.

Addressed review comments

Addressed review comments

H-G-Hristov added inline comments.Jun 1 2023, 2:01 PM
libcxx/test/support/test_container_comparisons.h
20

Cool! I looked for off-the-shelf containers but couldn't find them.
FYI The queue supports lists. I guess the stack supports vectors. I'll use it there.

H-G-Hristov marked an inline comment as done.Jun 1 2023, 2:01 PM

Cleanup and rebase

Mordante accepted this revision.Jun 3 2023, 6:58 AM

LGTM!

This revision is now accepted and ready to land.Jun 3 2023, 6:58 AM

As mentioned in D146094 these are the changes I applied locally to make friendship work. Can you test this too. If the CI fails please ping me on Discord then I will have another look.

libcxx/include/queue
411
496

Re-applied suggested changes: friend operator<=>

As mentioned in D146094 these are the changes I applied locally to make friendship work. Can you test this too. If the CI fails please ping me on Discord then I will have another look.

It seems this solution fails in Clang 16 and works in Clang 15 and Clang 17. Please leave a comment in the body of the operator<=> to explain why you use the current approach.

Rebased + fixed implementation

H-G-Hristov marked 2 inline comments as done.Jun 4 2023, 10:05 AM

@Mordante Thank you for looking into this.

This revision was automatically updated to reflect the committed changes.