Details

Reviewers

None

Group Reviewers

Restricted Project

Summary

Implements: P1132R8 - 20.3.4 Smart pointer adaptors

Diff Detail

Repository: rG LLVM Github Monorepo

Unit TestsFailed
View All

	Time	Test
	380 ms	libcxx-ci - GCC 13 / C++latest > llvm-libc++-shared-gcc-cfg-in.llvm-libc++-shared-gcc-cfg-in::/home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-1783479727a8-1/llvm-project/libcxx-ci/build/generic-gcc/test/libcxx/system_reserved_names.gen.py/memory.compile.pass.cpp Script: -- : 'COMPILED WITH'; /usr/bin/g++-13 /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-1783479727a8-1/llvm-project/libcxx-ci/build/generic-gcc/test/libcxx/system_reserved_names.gen.py/memory.compile.pass.cpp -pthread -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-1783479727a8-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-1783479727a8-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-1783479727a8-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-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-reserved-module-identifier -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 -Wno-local-type-template-args -Wno-c++11-extensions -Wno-unknown-pragmas -Wno-pass-failed -Wno-dangling-reference -Wno-mismatched-new-delete -Wno-redundant-move -Wno-self-move -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -fsyntax-only

Event Timeline

H-G-Hristov created this revision.May 14 2023, 5:31 AM

Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2023, 5:31 AM

Herald added a subscriber: arichardson. · View Herald Transcript

H-G-Hristov requested review of this revision.May 14 2023, 5:31 AM

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 14 2023, 5:31 AM

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

Herald added subscribers: llvm-commits, libcxx-commits, alextsao1999. · View Herald Transcript

Harbormaster completed remote builds in B231862: Diff 521998.May 14 2023, 7:23 AM

H-G-Hristov planned changes to this revision.May 14 2023, 7:31 AM

Try to fix CI

H-G-Hristov planned changes to this revision.May 14 2023, 7:53 AM

Harbormaster completed remote builds in B231865: Diff 522002.May 14 2023, 8:25 AM

Try to fix CI

H-G-Hristov planned changes to this revision.May 14 2023, 9:38 AM

Harbormaster completed remote builds in B231872: Diff 522011.May 14 2023, 11:08 AM

Try to fix CI

H-G-Hristov planned changes to this revision.May 14 2023, 9:41 PM

Harbormaster completed remote builds in B231892: Diff 522038.May 14 2023, 10:38 PM

Try to fix CI

Harbormaster completed remote builds in B231919: Diff 522066.May 15 2023, 1:43 AM

H-G-Hristov planned changes to this revision.May 15 2023, 1:48 AM

Rebased + Try to fix CI

H-G-Hristov planned changes to this revision.Jun 8 2023, 3:02 PM

philnik added a subscriber: philnik.Jun 8 2023, 4:31 PM

philnik added inline comments.

libcxx/include/__memory/inout_ptr.h
35	Maybe also `static_assert` Cpp17NullablePointer?
39–42	AFAICT this should be `if constexpr (is_pointer_v<_Smart>)`. Why do you check whether `s.get()` is valid?
63	This shouldn't use curly braces. Otherwise an implicit conversion is impossible. regression test!
72	Same here.
96–100
libcxx/include/__memory/out_ptr.h
33–35
39	Looks like you're not value-initializing `__p_`.
43	Isn't the assertion a bit redundant?
65	This isn't ill-formed in the else case!
86–90
libcxx/include/__memory/pointer_traits.h
277	Why is this implemented completely differently from the standard wording?
libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.compile.fail.cpp
1 ↗	(On Diff #529738)	This should be a `.verify.cpp`.

Harbormaster completed remote builds in B237596: Diff 529738.Jun 8 2023, 5:00 PM

@philnik Thank you very much for the review. I wanted to submit this as a draft to test it against the CI but by mistake I made it public.
I very much appreciate the feedback I'll try to find time to address the comments and add more tests soon.

H-G-Hristov edited the summary of this revision. (Show Details)Jun 9 2023, 8:05 AM

I've not looked at the patch, since changes are planed. I noticed a file that should not be modified.

llvm/utils/gn/secondary/libcxx/include/BUILD.gn
568 ↗	(On Diff #529738)	This file is automatically updated by a bot after updating this file.

Addressed some comments

WIP: Test CI

H-G-Hristov planned changes to this revision.Jun 27 2023, 8:55 AM

Harbormaster completed remote builds in B241505: Diff 535006.Jun 27 2023, 9:36 AM

Addressed comments

Harbormaster completed remote builds in B241865: Diff 535490.Jun 28 2023, 3:18 PM

@philnik I resolved most of the comments. I think the patch is ready for another round of reviews.

I believe named requirements need to be implemented as concepts. I am not convinced that my first iteration of Cpp17NullablePointer is entirely correct, so I hope for feedback.
The concepts and other type traits stuff which were added, should probably be refactored and moved to other locations too. In this or in another (follow-up) patch. Please suggest.

Thank you for the initial review.

libcxx/include/__memory/inout_ptr.h
63	Curly braces fixed, there is no test yet. TODO: Test
libcxx/include/__memory/out_ptr.h
65	Added a `static_assert` in the `else` path. I hope that's the correct approach.

H-G-Hristov marked an inline comment as done.Jun 28 2023, 10:36 PM

H-G-Hristov added inline comments.

libcxx/include/__iterator/cpp17_iterator_concepts.h
77	This concept and the other above should probably be moved to another location?
libcxx/include/__memory/allocator_traits.h
38	Should this be renamed and moved to a more central location so it can be reused?

Rebased

Re-run CI

Harbormaster completed remote builds in B256965: Diff 556409.Mon, Sep 11, 4:05 AM

gentle ping....

The CI error seems unrelated and appeared after the rebase.

Shall we continue with the review here or should I move this to GitHub. I am not entirely sure if I can move one patch or I need to move all of them.

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[libc++][memory] P1132R8: out_ptr - a scalable output pointer abstraction
Needs ReviewPublic
Actions

Details

Diff Detail

Unit TestsFailed
View All

Event Timeline

Revision Contents
Changeset List

Diff 556409

libcxx/docs/FeatureTestMacroTable.rst

libcxx/docs/Status/Cxx23Issues.csv

libcxx/docs/Status/Cxx23Papers.csv

libcxx/include/CMakeLists.txt

libcxx/include/__iterator/cpp17_iterator_concepts.h

libcxx/include/__memory/allocator_traits.h

libcxx/include/__memory/inout_ptr.h

libcxx/include/__memory/out_ptr.h

libcxx/include/__memory/pointer_traits.h

libcxx/include/memory

libcxx/include/module.modulemap.in

libcxx/include/version

libcxx/modules/std/memory.inc

libcxx/test/libcxx/memory/cpp17_nullable_pointer.compile.pass.cpp

libcxx/test/std/language.support/support.limits/support.limits.general/memory.version.compile.pass.cpp

libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.compile.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.t.compile.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.t.verify.cpp

libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.verify.cpp

libcxx/test/std/utilities/smartptr/adapt/out.ptr/out.ptr.compile.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/out.ptr/out.ptr.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/out.ptr/out.ptr.t.compile.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/out.ptr/out.ptr.t.verify.cpp

libcxx/test/std/utilities/smartptr/adapt/out.ptr/out.ptr.verify.cpp

libcxx/test/std/utilities/smartptr/adapt/types.h

libcxx/utils/generate_feature_test_macro_components.py

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[libc++][memory] P1132R8: out_ptr - a scalable output pointer abstractionNeeds ReviewPublicActions

Details

Diff Detail

Unit TestsFailedView All

Event Timeline

Revision ContentsChangeset List

Diff 556409

libcxx/docs/FeatureTestMacroTable.rst

libcxx/docs/Status/Cxx23Issues.csv

libcxx/docs/Status/Cxx23Papers.csv

libcxx/include/CMakeLists.txt

libcxx/include/__iterator/cpp17_iterator_concepts.h

libcxx/include/__memory/allocator_traits.h

libcxx/include/__memory/inout_ptr.h

libcxx/include/__memory/out_ptr.h

libcxx/include/__memory/pointer_traits.h

libcxx/include/memory

libcxx/include/module.modulemap.in

libcxx/include/version

libcxx/modules/std/memory.inc

libcxx/test/libcxx/memory/cpp17_nullable_pointer.compile.pass.cpp

libcxx/test/std/language.support/support.limits/support.limits.general/memory.version.compile.pass.cpp

libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.compile.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.t.compile.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.t.verify.cpp

libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.verify.cpp

libcxx/test/std/utilities/smartptr/adapt/out.ptr/out.ptr.compile.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/out.ptr/out.ptr.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/out.ptr/out.ptr.t.compile.pass.cpp

libcxx/test/std/utilities/smartptr/adapt/out.ptr/out.ptr.t.verify.cpp

libcxx/test/std/utilities/smartptr/adapt/out.ptr/out.ptr.verify.cpp

libcxx/test/std/utilities/smartptr/adapt/types.h

libcxx/utils/generate_feature_test_macro_components.py

[libc++][memory] P1132R8: out_ptr - a scalable output pointer abstraction
Needs ReviewPublic
Actions

Unit TestsFailed
View All

Revision Contents
Changeset List