This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by H-G-Hristov on Mar 14 2023, 2:28 PM.

Details

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

Depends on D146066

Depends on D132268

Implements parts of P1614R2 operator<=> for stack

Diff Detail

Event Timeline

H-G-Hristov created this revision.Mar 14 2023, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 2:28 PM
Herald added a subscriber: yaxunl. · View Herald Transcript
H-G-Hristov requested review of this revision.Mar 14 2023, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 2:28 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
  • Added URL
H-G-Hristov edited the summary of this revision. (Show Details)Mar 14 2023, 2:53 PM
H-G-Hristov edited the summary of this revision. (Show Details)
H-G-Hristov edited the summary of this revision. (Show Details)
H-G-Hristov planned changes to this revision.Mar 22 2023, 2:39 PM
H-G-Hristov planned changes to this revision.May 29 2023, 6:01 AM

Addressed review comments

Rebased + Fix CI

Mordante accepted this revision.Jun 3 2023, 7:03 AM
Mordante added a subscriber: Mordante.

LGTM after addressing the review comments.

libcxx/include/stack
106

I miss an update of SpaceshipProjects.csv

411

If possible we prefer to do exactly what the Standard says. This is consistent with operator== and operator<.

This revision is now accepted and ready to land.Jun 3 2023, 7:03 AM
H-G-Hristov added a comment.EditedJun 3 2023, 10:48 AM

@Mordante Thank you for the review.

libcxx/include/stack
411

c is a protected member of stack, which requires to declare the operator<=> as a friend similar to operator== and operator<. Such implementation compiles fine with Apple Clang 14.0.3 (LLVM 15) but it fails to compile on the CI with newer Clang 16/17 (I think) with myriad of ambiguous operator declarations errors all over the place. I did some research but all of this was over my head, so I settled on the above implementation.
I did the same for queue. I'll refrain to land these patches until I get more feedback.

Mordante added inline comments.Jun 4 2023, 3:32 AM
libcxx/include/stack
411

I really would like to know what the issue with friends are. I tested with queue and it works for me. I will post more info on that patch.

Rebased + addressed comments

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

@Mordante Thank you for looking into this. I left a comment according to https://reviews.llvm.org/D146066 and I marked the comments as Done.
I'll rebase the patch later to get the CI green again.

This revision was automatically updated to reflect the committed changes.