⚙ D135548 [libc++][LWG 2996] Implement c++20 shared

pateldeev created this revision.Oct 9 2022, 5:32 PM

Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2022, 5:32 PM

pateldeev edited the summary of this revision. (Show Details)Oct 9 2022, 5:36 PM

pateldeev published this revision for review.Oct 9 2022, 5:38 PM

pateldeev added a reviewer: philnik.

Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2022, 5:39 PM

Herald added a reviewer: Restricted Project. · View Herald Transcript

Herald added a subscriber: libcxx-commits. · View Herald Transcript

pateldeev edited the summary of this revision. (Show Details)Oct 9 2022, 5:39 PM

Harbormaster completed remote builds in B191196: Diff 466403.Oct 9 2022, 6:23 PM

variable name.

Harbormaster completed remote builds in B191209: Diff 466418.Oct 9 2022, 9:27 PM

Unit tests.

Harbormaster completed remote builds in B191210: Diff 466420.Oct 9 2022, 10:13 PM

Fix

Harbormaster completed remote builds in B191211: Diff 466421.Oct 9 2022, 10:51 PM

Unit tests.

new line.

Harbormaster completed remote builds in B191220: Diff 466430.Oct 10 2022, 12:37 AM

Thanks for working on this. In general looks quite good, but some code uses an older style we don't use for new code.

libcxx/docs/Status/Cxx20Issues.csv
105
libcxx/include/__memory/shared_ptr.h
632	Please update the synopsis too.
632	We (and other vendors) typically backport LWG-issue, can you do that for this issue too.
634
722	Please undo this change. In the past we used `_VSTD` instead of `std`. We don't change all existing code to avoid breaking too much work in progress, but we use it when changing code.
1521	`_LIBCPP_HIDE_FROM_ABI` is the new name for `_LIBCPP_INLINE_VISIBILITY`.
1522
1540	See above.
1541	In new code we prefer `using` over `typedef`.
1577	The extra space isn't fixed by clang-format due to C++03 compatibility.
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/const_pointer_cast.pass.cpp
14	Please test whether this is really noexcept. There is a `ASSERT_NOEXCEPT` helper macro. The same for the other tests.

This revision now requires changes to proceed.Oct 12 2022, 11:31 AM

pateldeev marked 9 inline comments as done.Oct 12 2022, 5:45 PM

pateldeev added inline comments.

libcxx/include/__memory/shared_ptr.h
632	I think back-porting to c++17 will break code that does a `std::move()` but relies on the fact that no move happens. Arguable this a just a bug in user code, but probably still something we don't want to change?

Review

Harbormaster completed remote builds in B191863: Diff 467322.Oct 12 2022, 5:46 PM

Review

Harbormaster completed remote builds in B191864: Diff 467324.Oct 12 2022, 9:03 PM

Mordante added a subscriber: jwakely.Oct 17 2022, 9:27 AM

Mordante added inline comments.

libcxx/include/__memory/shared_ptr.h
632	I don't feel that we should guard against users doing bad things; updating their code to C++20 will break that scenario. I see MSVC STL applied this change unconditionally, but libstdc++ limits it to C++20 and newer. @jwakely is there a reason for not backporting LWG-2996 to older versions?
632	Or remove it entirely.
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/const_pointer_cast.pass.cpp
69	Or removed, the same for other tests.

jwakely added inline comments.Oct 17 2022, 2:18 PM

libcxx/include/__memory/shared_ptr.h
632	I don't remember why I only added it for C++20, but probably because it's questionable whether there was a defect. It seems more like an evolutionary change (which is why it went through LEWG) than a defect fix.

Mordante added inline comments.Oct 18 2022, 11:18 AM

libcxx/include/__memory/shared_ptr.h
632	I don't remember why I only added it for C++20, but probably because it's questionable whether there was a defect. It seems more like an evolutionary change (which is why it went through LEWG) than a defect fix. Thanks for the information. Then it makes sense to me to do the same in libc++. @pateldeev please leave a comment why this LWG issue isn't backported to earlier language versions.

pateldeev marked 7 inline comments as done.Oct 18 2022, 2:26 PM

Comments

pateldeev edited the summary of this revision. (Show Details)Oct 18 2022, 2:32 PM

Harbormaster completed remote builds in B192853: Diff 468702.Oct 18 2022, 4:02 PM

pateldeev removed reviewers: philnik, Restricted Project, Mordante.Nov 13 2022, 11:43 PM

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 13 2022, 11:43 PM

pateldeev added reviewers: philnik, Mordante.Nov 13 2022, 11:45 PM

@philnik Can you have a pass at this?

Please mention the LWG issue you are implementing in the title. Other than that and inline comments this LGTM. Leaving final approval to @Mordante, since he had comments.

libcxx/include/__memory/shared_ptr.h
634	The LWG issue is against C++20. Same for the other changes.
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/const_pointer_cast.pass.cpp
69
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_pointer.pass.cpp
106–115	I'd just remove this test. I don't really see the point of testing this, since it's questionable to rely on this behaviour and it's been changed with an LWG issue which some implementations back-port.

pateldeev retitled this revision from [libc++] Implement c++20 shared_ptr rvalue overloads. to [libc++][LWG 2996] Implement c++20 shared_ptr rvalue overloads..Jan 14 2023, 4:52 PM

pateldeev marked an inline comment as done.

Review. Rebase

pateldeev marked an inline comment as done.Jan 14 2023, 5:17 PM

pateldeev removed a reviewer: Restricted Project.Jan 14 2023, 5:20 PM

This revision is now accepted and ready to land.Jan 14 2023, 5:20 PM

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 14 2023, 5:20 PM

This revision now requires review to proceed.Jan 14 2023, 5:20 PM

Harbormaster completed remote builds in B207878: Diff 489319.Jan 14 2023, 7:51 PM

Thanks for the change, LGTM modulo comments and green CI (please rebase first). Thanks!

libcxx/docs/Status/Cxx20Issues.csv
105

This revision is now accepted and ready to land.Feb 9 2023, 8:37 AM

pateldeev marked an inline comment as done.Feb 16 2023, 10:20 PM

Rebase

Rebased diff and addressed all comments. Should be ready to merge. But it seems I don't have the ability to do this.

@ldionne: Can you help merge this?

Harbormaster completed remote builds in B214326: Diff 498251.Feb 16 2023, 11:55 PM

[libc++][LWG 2996] Implement c++20 shared_ptr rvalue overloads.
AcceptedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 498251

libcxx/docs/Status/Cxx20Issues.csv

libcxx/include/__memory/shared_ptr.h

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/const_pointer_cast.pass.cpp

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/dynamic_pointer_cast.pass.cpp

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/reinterpret_pointer_cast.pass.cpp

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/static_pointer_cast.pass.cpp

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_pointer.pass.cpp

[libc++][LWG 2996] Implement c++20 shared_ptr rvalue overloads.AcceptedPublicActions