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

[pateldeev]

Summary

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

Commit from
"patedeev" <[e-mail]>

[pateldeev]

variable name.

[pateldeev]

Unit tests.

[pateldeev]

Fix

[pateldeev]

Unit tests.

[pateldeev]

new line.

[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

594

Please update the synopsis too.

594

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

596

681

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.

1388

_LIBCPP_HIDE_FROM_ABI is the new name for _LIBCPP_INLINE_VISIBILITY.

1389

1405

See above.

1406

In new code we prefer using over typedef.

1441

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.

[pateldeev]

libcxx/include/__memory/shared_ptr.h

594

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?

[pateldeev]

Review

[pateldeev]

Review

[Mordante]

libcxx/include/__memory/shared_ptr.h

594

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?

594

Or remove it entirely.

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

68

Or removed, the same for other tests.

[jwakely]

libcxx/include/__memory/shared_ptr.h

594

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]

libcxx/include/__memory/shared_ptr.h

594

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]

Comments

[ldionne]

@philnik Can you have a pass at this?

[philnik]

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

596

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

68

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

116–127 ↗

(On Diff #468702)

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]

Review. Rebase

[ldionne]

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

libcxx/docs/Status/Cxx20Issues.csv

105

[pateldeev]

Rebase

[pateldeev]

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?

[Mordante]

In D135548#4134054, @pateldeev wrote:

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" <[e-mail]> then we can commit it on your behalf.

[pateldeev]

Rebase

[pateldeev]

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

Done

Can you also provide "user name" <[e-mail]> then we can commit it on your behalf.

Added to bottom of diff description