Page MenuHomePhabricator

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

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

Details

Reviewers
philnik
Mordante
ldionne
pateldeev
Group Reviewers
Restricted Project
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

Unit TestsFailed

TimeTest
810 mslibcxx CI C++2b > llvm-libc++-shared-cfg-in.std/utilities/memory/util_smartptr/util_smartptr_shared/util_smartptr_shared_cast::const_pointer_cast.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-16 /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/const_pointer_cast.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/Output/const_pointer_cast.pass.cpp.dir/t.tmp.exe
710 mslibcxx CI C++2b > llvm-libc++-shared-cfg-in.std/utilities/memory/util_smartptr/util_smartptr_shared/util_smartptr_shared_cast::reinterpret_pointer_cast.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-16 /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/reinterpret_pointer_cast.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/Output/reinterpret_pointer_cast.pass.cpp.dir/t.tmp.exe
860 mslibcxx CI C++2b > llvm-libc++-shared-cfg-in.std/utilities/memory/util_smartptr/util_smartptr_shared/util_smartptr_shared_cast::static_pointer_cast.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-16 /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/static_pointer_cast.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/Output/static_pointer_cast.pass.cpp.dir/t.tmp.exe
900 mslibcxx CI C++2b > llvm-libc++-shared-cfg-in.std/utilities/memory/util_smartptr/util_smartptr_shared/util_smartptr_shared_const::shared_ptr_pointer.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-16 /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_pointer.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/2a4686310657-1/llvm-project/libcxx-ci/build/generic-cxx2b/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/Output/shared_ptr_pointer.pass.cpp.dir/t.tmp.exe
720 mslibcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.std/utilities/memory/util_smartptr/util_smartptr_shared/util_smartptr_shared_cast::const_pointer_cast.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/87caf32fb7e2-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/const_pointer_cast.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/87caf32fb7e2-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/87caf32fb7e2-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/87caf32fb7e2-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -lc++experimental -L /home/libcxx-builder/.buildkite-agent/builds/87caf32fb7e2-1/llvm-project/libcxx-ci/build/generic-gcc/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/87caf32fb7e2-1/llvm-project/libcxx-ci/build/generic-gcc/lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/87caf32fb7e2-1/llvm-project/libcxx-ci/build/generic-gcc/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/Output/const_pointer_cast.pass.cpp.dir/t.tmp.exe
View Full Test Results (12 Failed)

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 Transcript
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
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.

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
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?

Mordante added inline comments.
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 added inline comments.Oct 17 2022, 2:18 PM
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 added inline comments.Oct 18 2022, 11:18 AM
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 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
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 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.EditedWed, May 24, 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)Wed, May 24, 7:44 PM