Page MenuHomePhabricator

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

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

Authored by H-G-Hristov on May 14 2023, 5:31 AM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

Diff Detail

Unit TestsFailed

TimeTest
380 mslibcxx-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
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

Try to fix CI

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

Try to fix CI

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

Try to fix CI

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.

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

H-G-Hristov edited the summary of this revision. (Show Details)

Addressed some comments

WIP: Test CI

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

Addressed comments

H-G-Hristov marked 10 inline comments as done.Jun 28 2023, 10:35 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?

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.