This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by avogelsgesang on Jul 31 2022, 12:43 PM.

Details

Summary

Implements part of:

  • P1614R2 The Mothership has Landed

Fixes LWG3427

Diff Detail

Event Timeline

avogelsgesang created this revision.Jul 31 2022, 12:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2022, 12:43 PM
avogelsgesang requested review of this revision.Jul 31 2022, 12:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2022, 12:43 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks for your contribution! When resolving the review comments please look whether they apply to your other patches too. (I first want to look at this patch, once approved I'll look at the others.)

For the CI, feel free to ignore the error of the format CI, this is a soft-error at the moment.

libcxx/docs/ReleaseNotes.rst
44 ↗(On Diff #448878)

We usually only mention "larger" improvements here not every subpart of a paper.

libcxx/include/__memory/shared_ptr.h
1241
1242

The macro only needs to be used in code that works in C++03.

1242

please use __x and __y for consistency.

1353

I see this matches the Wording in the Standard, but it differs from the paper. Does this complete an LWG issue?

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp
48

Partly pre-existing, but I miss tests to validate the return type and that the function is noexcept. Can you add tests for this? See D128603 for examples.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/order.pass.cpp
8–9 ↗(On Diff #448878)
24 ↗(On Diff #448878)

Please remove this #if the UNSUPPORTED is the libc++ way to achieve this behaviour.

32 ↗(On Diff #448878)

Please test all equality and comparison operators.

avogelsgesang marked 6 inline comments as done.

Address comments by @Mordante

Thanks for your contribution!

Thank you for your review!

When resolving the review comments please look whether they apply to your other patches too. (I first want to look at this patch, once approved I'll look at the others.)

I will do so. I will let you know as soon as the other reviews are updated (probably only after we land this one, so I don't have to update the other commits multiple times)

libcxx/docs/ReleaseNotes.rst
44 ↗(On Diff #448878)

works for me - that also gets rid of the annoying merge conflict on this line which I will have with all of the other posted reviews

libcxx/include/__memory/shared_ptr.h
1353
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp
48

Added calls to AssertComparisonsAreNoexcept, AssertComparisonsReturnBool, AssertOrderAreNoexcept, etc.

My code required additional #ifdefs, though.
Would it make sense to introduce AssertOrderReturn also for dialects < C++20, and let it gracefully degrade to AssertComparisonsReturnBool on those earlier C++ versions?

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/order.pass.cpp
32 ↗(On Diff #448878)

Those were already tested in lt.pass.cpp and eq.pass.cpp.
I do agree, though, that the splitting across multiple files made it hard to judge the existing test coverage. Hence, I merged this test with the pre-existing lt.pass.cpp and eq.pass.cpp.
I think doing so improves readability of this test case.

avogelsgesang edited the summary of this revision. (Show Details)Aug 1 2022, 12:26 PM
avogelsgesang edited the summary of this revision. (Show Details)

Forgot to git add the _LIBCPP_HIDE_FROM_ABI

Fix templates so they also build on C++03

add missing include

In general I'm happy, one nit remaining. I like to see the CI green before approving.

libcxx/include/__memory/shared_ptr.h
1242

Seems you missed the noexcept here, but I see it's fixed at the other location.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp
48

No I think that doesn't make sense since it changes the macro depending on the language version used. In libc++ and its tests we're used to using #if often. Sometimes we have some helper macros, but only for generic things like CONSTEXPER_AFTER_XX.

avogelsgesang marked 2 inline comments as done.
  • Use noexcept instead of macro
  • Rebase on D130855
  • (Hopefully) fix CI by adding missing include (?)

Status: Done -> Complete

Mordante accepted this revision.Aug 3 2022, 9:30 AM

Thanks, LGTM!
I'll commit this on your behalf.

This revision is now accepted and ready to land.Aug 3 2022, 9:30 AM
This revision was automatically updated to reflect the committed changes.
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/eq.pass.cpp