This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by pateldeev on Oct 9 2022, 5:32 PM.

Details

Summary

Implement c++20 shared_ptr rvalue overloads for aliasing constructor and pointer casts. See https://cplusplus.github.io/LWG/issue2996

Commit from
"patedeev" <dkp10000@gmail.com>

Diff Detail

Event Timeline

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 TranscriptOct 9 2022, 5:39 PM
pateldeev edited the summary of this revision. (Show Details)Oct 9 2022, 5:39 PM
pateldeev updated this revision to Diff 466418.Oct 9 2022, 8:57 PM

variable name.

pateldeev updated this revision to Diff 466420.Oct 9 2022, 10:13 PM

Unit tests.

pateldeev updated this revision to Diff 466429.Oct 9 2022, 11:10 PM

Unit tests.

Mordante requested changes to this revision.Oct 12 2022, 11:31 AM
Mordante added a subscriber: Mordante.

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
647

Please update the synopsis too.

647

We (and other vendors) typically backport LWG-issue, can you do that for this issue too.

649
737

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.

1536

_LIBCPP_HIDE_FROM_ABI is the new name for _LIBCPP_INLINE_VISIBILITY.

1537
1555

See above.

1556

In new code we prefer using over typedef.

1592

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
647

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?

Mordante added inline comments.
libcxx/include/__memory/shared_ptr.h
647

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?

647

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
647

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
647

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
pateldeev edited the summary of this revision. (Show Details)Oct 18 2022, 2:32 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

@philnik Can you have a pass at this?

philnik accepted this revision as: philnik.Jan 2 2023, 7:32 AM

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
649

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.
pateldeev marked an inline comment as done.
pateldeev updated this revision to Diff 489319.Jan 14 2023, 5:16 PM

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
ldionne accepted this revision.Feb 9 2023, 8:37 AM

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
pateldeev accepted this revision.EditedFeb 16 2023, 10:29 PM

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?

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?

Could you rebased this again and upload it to test the CI.
Can you also provide "user name" <email@my.address> then we can commit it on your behalf.

pateldeev added a comment.EditedMay 24 2023, 7:44 PM

Could you rebased this again and upload it to test the CI.

Done

Can you also provide "user name" <email@my.address> then we can commit it on your behalf.

Added to bottom of diff description

pateldeev edited the summary of this revision. (Show Details)May 24 2023, 7:44 PM

Can you rebase this patch to see whether it passes the CI?

cc @Mordante to merge.

Thanks I can now pick it without conflicts. I noticed the attribution is "pateldeev <pateldeev@nevada.unr.edu>" Can you provide they name you want me to attribute this patch too or confirm "pateldeev" is intended?

pateldeev added a comment.EditedJul 18 2023, 9:00 AM

cc @Mordante to merge.

Thanks I can now pick it without conflicts. I noticed the attribution is "pateldeev <pateldeev@nevada.unr.edu>" Can you provide they name you want me to attribute this patch too or confirm "pateldeev" is intended?

That attribution works. No need to update. Thanks

cc @Mordante to merge.

Thanks I can now pick it without conflicts. I noticed the attribution is "pateldeev <pateldeev@nevada.unr.edu>" Can you provide they name you want me to attribute this patch too or confirm "pateldeev" is intended?

That attribution works. No need to update. Thanks

Thanks, I'll land it on your behalf.