This is an archive of the discontinued LLVM Phabricator instance.

Differential D144821

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

Authored by H-G-Hristov on Feb 26 2023, 2:03 AM.

Details

Reviewers
avogelsgesang
Mordante
philnik
Group Reviewers
Restricted Project
Commits
rG2ff646f554bc: [libc++][spaceship] Implement `operator<=>` for `deque`
Summary

Based on https://reviews.llvm.org/D132312
Dependes on https://reviews.llvm.org/D132312

Diff Detail

Event Timeline

H-G-Hristov created this revision.Feb 26 2023, 2:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2023, 2:03 AM
H-G-Hristov requested review of this revision.Feb 26 2023, 2:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2023, 2:03 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: libcxx-commits. · View Herald Transcript
H-G-Hristov abandoned this revision.Feb 26 2023, 2:52 AM
H-G-Hristov updated this revision to Diff 500534.Feb 26 2023, 3:01 AM

Updated papers status

H-G-Hristov abandoned this revision.Feb 26 2023, 3:02 AM
Harbormaster completed remote builds in B216029: Diff 500534.Feb 26 2023, 5:02 AM
H-G-Hristov updated this revision to Diff 500550.Feb 26 2023, 5:10 AM
  • [libc++][spaceship] Implement operator<=> for deque
  • Removed test_container_comparisons.h

Based on https://reviews.llvm.org/D132312

H-G-Hristov edited the summary of this revision. (Show Details)Feb 26 2023, 5:11 AM
H-G-Hristov retitled this revision from Updated: Cxx2bPapers.csv to [libc++][spaceship] Implement `operator<=>` for `deque`.
Herald added a subscriber: yaxunl. · View Herald TranscriptFeb 26 2023, 5:11 AM
H-G-Hristov added a reviewer: avogelsgesang.Feb 26 2023, 5:12 AM
H-G-Hristov edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B216042: Diff 500550.Feb 26 2023, 7:18 AM
avogelsgesang added a parent revision: D132312: [libc++][spaceship] Implement `operator<=>` for `list`.Feb 27 2023, 3:41 PM
avogelsgesang added a reviewer: Mordante.Feb 27 2023, 3:49 PM
avogelsgesang added a subscriber: Mordante.

Thanks for this change!

In general, I won't be able to approve your commit for the libc++ group. In the end, you will need the review from one of the "core libc++ contributors" (e.g., @Mordante) to ship this fix.

Also, for "stacked commits", i.e. commits which depend on other changes which didn't land on main, yet, you need to add a parent revision here on Phabricator. I did this for you already and also restarted the CI job. So this is just for future reference :)

Please also update libcxx/docs/Status/SpaceshipProjects.csv

libcxx/include/deque
2398 ↗(On Diff #500550)

We need to ADL-proof this. My other commit for std::list was actually also missing this. A clang-tidy check in our CI should catch this and report an error for this line...

libcxx/test/libcxx/containers/sequences/deque/compare.three_way.pass.cpp
26 ↗(On Diff #500550)

missing newline at end of file

(I assume you are on Windows? In case you are using VS Code, there is an "Insert Final Newline" setting which you might want to change)

avogelsgesang added inline comments.Feb 27 2023, 3:55 PM
libcxx/test/libcxx/containers/sequences/deque/compare.three_way.pass.cpp
10 ↗(On Diff #500550)
H-G-Hristov updated this revision to Diff 501430.Mar 1 2023, 2:12 AM
  • [libc++][spaceship] Implement operator<=> for deque
  • Removed test_container_comparisons.h
  • Addressed code reviews
  • Restored
H-G-Hristov marked 3 inline comments as done.Mar 1 2023, 2:21 AM
In D144821#4156977, @avogelsgesang wrote:

Thanks for this change!

In general, I won't be able to approve your commit for the libc++ group. In the end, you will need the review from one of the "core libc++ contributors" (e.g., @Mordante) to ship this fix.

Also, for "stacked commits", i.e. commits which depend on other changes which didn't land on main, yet, you need to add a parent revision here on Phabricator. I did this for you already and also restarted the CI job. So this is just for future reference :)

Please also update libcxx/docs/Status/SpaceshipProjects.csv

Thank you very much for the review and the notes and of course for completing the real work previously. I wanted to try out the process of submitting a patch.

A have a couple of questions:

  1. Do I need to add anyone specific as a reviewer, message/ask somebody or I should just wait.
  2. If this patch passes the review successfully, would you mind if I try completing some of the other containers too?
H-G-Hristov added inline comments.Mar 1 2023, 2:24 AM
libcxx/test/libcxx/containers/sequences/deque/compare.three_way.pass.cpp
26 ↗(On Diff #500550)

I don't get it. Is this file still missing an empty line?

Harbormaster completed remote builds in B216673: Diff 501430.Mar 1 2023, 3:38 AM
philnik accepted this revision.Mar 1 2023, 7:46 AM
philnik added a subscriber: philnik.
In D144821#4161020, @H-G-Hristov wrote:
In D144821#4156977, @avogelsgesang wrote:

Thanks for this change!

In general, I won't be able to approve your commit for the libc++ group. In the end, you will need the review from one of the "core libc++ contributors" (e.g., @Mordante) to ship this fix.

Also, for "stacked commits", i.e. commits which depend on other changes which didn't land on main, yet, you need to add a parent revision here on Phabricator. I did this for you already and also restarted the CI job. So this is just for future reference :)

Please also update libcxx/docs/Status/SpaceshipProjects.csv

Thank you very much for the review and the notes and of course for completing the real work previously. I wanted to try out the process of submitting a patch.

A have a couple of questions:

  1. Do I need to add anyone specific as a reviewer, message/ask somebody or I should just wait.

For libc++ not, because #libc gets automatically added, so every libc++ approver gets notified. For other subprojects that's not necessarily the case, so you might have to add people.

  1. If this patch passes the review successfully, would you mind if I try completing some of the other containers too?

Feel free to do so, but make sure there isn't a patch already (AFAICT for spaceship there is only for array and vector a patch). More contributors are always welcome.

LGTM % nits. I guess you don't have commit access? In that case, please provide "Your Name" <your.email@address>.

libcxx/include/deque
146–149 ↗(On Diff #501430)
libcxx/test/libcxx/containers/sequences/deque/compare.three_way.pass.cpp
16–17 ↗(On Diff #501430)
26 ↗(On Diff #500550)

No. Phab only complains about it when there isn't one.

This revision is now accepted and ready to land.Mar 1 2023, 7:46 AM
H-G-Hristov updated this revision to Diff 501548.Mar 1 2023, 9:20 AM

Addressed review comments

H-G-Hristov marked an inline comment as done.Mar 1 2023, 9:22 AM
Harbormaster completed remote builds in B216746: Diff 501548.Mar 1 2023, 10:51 AM
Mordante accepted this revision.Mar 1 2023, 10:52 AM
In D144821#4161664, @philnik wrote:
In D144821#4161020, @H-G-Hristov wrote:

not necessarily the case, so you might have to add people.

  1. If this patch passes the review successfully, would you mind if I try completing some of the other containers too?

Feel free to do so, but make sure there isn't a patch already (AFAICT for spaceship there is only for array and vector a patch). More contributors are always welcome.

Feel free to pick the items that are unassigned. When you want to pick an assigned item, best discuss on Discord. Some of the assigned items may have a patch already.

Thanks for your contribution, LGTM!

H-G-Hristov updated this revision to Diff 501928.Mar 2 2023, 11:00 AM
  • Addressed comment
H-G-Hristov marked an inline comment as done.Mar 2 2023, 11:04 AM

@philnik @Mordante Thank you for your feedback!

Here is my name and email:

Hristo Hristov <zingam@outlook.com>

Harbormaster completed remote builds in B217016: Diff 501928.Mar 2 2023, 12:29 PM
Closed by commit rG2ff646f554bc: [libc++][spaceship] Implement `operator<=>` for `deque` (authored by Zingam, committed by philnik). · Explain WhyMar 3 2023, 4:36 AM
This revision was automatically updated to reflect the committed changes.
philnik added a commit: rG2ff646f554bc: [libc++][spaceship] Implement `operator<=>` for `deque`.

Revision Contents

PathSize
libcxx/
docs/
Status/
2 lines

Diff 500527

libcxx/docs/Status/Cxx2bPapers.csv

Show First 20 Lines • Show All 102 Lines • ▼ Show 20 Lines
"`P2505R5 <https://wg21.link/P2505R5>`__","LWG", "Monadic Functions for ``std::expected``", "November 2022","","","" "`P2505R5 <https://wg21.link/P2505R5>`__","LWG", "Monadic Functions for ``std::expected``", "November 2022","","",""
"`P2539R4 <https://wg21.link/P2539R4>`__","LWG", "Should the output of ``std::print`` to a terminal be synchronized with the underlying stream?", "November 2022","","","|format|" "`P2539R4 <https://wg21.link/P2539R4>`__","LWG", "Should the output of ``std::print`` to a terminal be synchronized with the underlying stream?", "November 2022","","","|format|"
"`P2602R2 <https://wg21.link/P2602R2>`__","LWG", "Poison Pills are Too Toxic", "November 2022","","","|ranges|" "`P2602R2 <https://wg21.link/P2602R2>`__","LWG", "Poison Pills are Too Toxic", "November 2022","","","|ranges|"
"`P2708R1 <https://wg21.link/P2708R1>`__","LWG", "No Further Fundamentals TSes", "November 2022","|Nothing to do|","","" "`P2708R1 <https://wg21.link/P2708R1>`__","LWG", "No Further Fundamentals TSes", "November 2022","|Nothing to do|","",""
"","","","","","","" "","","","","","",""
"`P0290R4 <https://wg21.link/P0290R4>`__","LWG", "``apply()`` for ``synchronized_value<T>``","February 2023","","","|concurrency TS|" "`P0290R4 <https://wg21.link/P0290R4>`__","LWG", "``apply()`` for ``synchronized_value<T>``","February 2023","","","|concurrency TS|"
"`P2770R0 <https://wg21.link/P2770R0>`__","LWG", "Stashing stashing ``iterators`` for proper flattening","February 2023","","","|ranges|" "`P2770R0 <https://wg21.link/P2770R0>`__","LWG", "Stashing stashing ``iterators`` for proper flattening","February 2023","","","|ranges|"
"`P2164R9 <https://wg21.link/P2164R9>`__","LWG", "``views::enumerate``","February 2023","","","|ranges|" "`P2164R9 <https://wg21.link/P2164R9>`__","LWG", "``views::enumerate``","February 2023","","","|ranges|"
"`P2711R1 <https://wg21.link/P2711R1>`__","LWG", "Making multi-param constructors of ``views`` ``explicit``","February 2023","","","|ranges|" "`P2711R1 <https://wg21.link/P2711R1>`__","LWG", "Making multi-param constructors of ``views`` ``explicit``","February 2023","Partial","","|ranges|"
"`P2609R3 <https://wg21.link/P2609R3>`__","LWG", "Relaxing Ranges Just A Smidge","February 2023","","","|ranges|" "`P2609R3 <https://wg21.link/P2609R3>`__","LWG", "Relaxing Ranges Just A Smidge","February 2023","","","|ranges|"
"`P2713R1 <https://wg21.link/P2713R1>`__","LWG", "Escaping improvements in ``std::format``","February 2023","","","|format|" "`P2713R1 <https://wg21.link/P2713R1>`__","LWG", "Escaping improvements in ``std::format``","February 2023","","","|format|"
"`P2675R1 <https://wg21.link/P2675R1>`__","LWG", "``format``'s width estimation is too approximate and not forward compatible","February 2023","","","|format|" "`P2675R1 <https://wg21.link/P2675R1>`__","LWG", "``format``'s width estimation is too approximate and not forward compatible","February 2023","","","|format|"
"`P2572R1 <https://wg21.link/P2572R1>`__","LWG", "``std::format`` fill character allowances","February 2023","","","|format|" "`P2572R1 <https://wg21.link/P2572R1>`__","LWG", "``std::format`` fill character allowances","February 2023","","","|format|"
"`P2693R1 <https://wg21.link/P2693R1>`__","LWG", "Formatting ``thread::id`` and ``stacktrace``","February 2023","","","|format|" "`P2693R1 <https://wg21.link/P2693R1>`__","LWG", "Formatting ``thread::id`` and ``stacktrace``","February 2023","","","|format|"
"`P2679R2 <https://wg21.link/P2679R2>`__","LWG", "Fixing ``std::start_lifetime_as`` for arrays","February 2023","","","" "`P2679R2 <https://wg21.link/P2679R2>`__","LWG", "Fixing ``std::start_lifetime_as`` for arrays","February 2023","","",""
"`P2674R1 <https://wg21.link/P2674R1>`__","LWG", "A trait for implicit lifetime types","February 2023","","","" "`P2674R1 <https://wg21.link/P2674R1>`__","LWG", "A trait for implicit lifetime types","February 2023","","",""
"`P2655R3 <https://wg21.link/P2655R3>`__","LWG", "``common_reference_t`` of ``reference_wrapper`` Should Be a Reference Type","February 2023","","","" "`P2655R3 <https://wg21.link/P2655R3>`__","LWG", "``common_reference_t`` of ``reference_wrapper`` Should Be a Reference Type","February 2023","","",""
"`P2652R2 <https://wg21.link/P2652R2>`__","LWG", "Disallow User Specialization of ``allocator_traits``","February 2023","","","" "`P2652R2 <https://wg21.link/P2652R2>`__","LWG", "Disallow User Specialization of ``allocator_traits``","February 2023","","",""
"`P2787R1 <https://wg21.link/P2787R1>`__","LWG", "``pmr::generator`` - Promise Types are not Values","February 2023","","","" "`P2787R1 <https://wg21.link/P2787R1>`__","LWG", "``pmr::generator`` - Promise Types are not Values","February 2023","","",""
"`P2614R2 <https://wg21.link/P2614R2>`__","LWG", "Deprecate ``numeric_limits::has_denorm``","February 2023","","","" "`P2614R2 <https://wg21.link/P2614R2>`__","LWG", "Deprecate ``numeric_limits::has_denorm``","February 2023","","",""
"`P2588R3 <https://wg21.link/P2588R3>`__","LWG", "``barrier``’s phase completion guarantees","February 2023","","","" "`P2588R3 <https://wg21.link/P2588R3>`__","LWG", "``barrier``’s phase completion guarantees","February 2023","","",""
"`P2763R1 <https://wg21.link/P2763R1>`__","LWG", "``layout_stride`` static extents default constructor fix","February 2023","","","" "`P2763R1 <https://wg21.link/P2763R1>`__","LWG", "``layout_stride`` static extents default constructor fix","February 2023","","",""
"`P2736R2 <https://wg21.link/P2736R2>`__","CWG","Referencing The Unicode Standard","February 2023","","","|format|" "`P2736R2 <https://wg21.link/P2736R2>`__","CWG","Referencing The Unicode Standard","February 2023","","","|format|"