Implements: P1132R8 - 20.3.4 Smart pointer adaptors
Details
- Reviewers
- None
- Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
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
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.
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. |
@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. | |
libcxx/include/__memory/out_ptr.h | ||
65 | Added a static_assert in the else path. I hope that's the correct approach. |
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.