Implements: P1132R8 - 20.3.4 Smart pointer adaptors
Details
- Reviewers
- None
- Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__memory/inout_ptr.h | ||
---|---|---|
34 | Maybe also static_assert Cpp17NullablePointer? | |
38–41 | AFAICT this should be if constexpr (is_pointer_v<_Smart>). Why do you check whether s.get() is valid? | |
62 | This shouldn't use curly braces. Otherwise an implicit conversion is impossible. regression test! | |
71 | Same here. | |
95–99 | ||
libcxx/include/__memory/out_ptr.h | ||
32–34 | ||
38 | Looks like you're not value-initializing __p_. | |
42 | Isn't the assertion a bit redundant? | |
64 | This isn't ill-formed in the else case! | |
85–89 | ||
libcxx/include/__memory/pointer_traits.h | ||
270 | Why is this implemented completely differently from the standard wording? | |
libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.compile.fail.cpp | ||
1 | 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 | 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 | ||
---|---|---|
62 | Curly braces fixed, there is no test yet. | |
libcxx/include/__memory/out_ptr.h | ||
64 | 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.
libcxx/include/__memory/inout_ptr.h | ||
---|---|---|
34 | I made an attempt to implement this requirement but I don't think it was anywhere near correct, so I removed it from this revision. I'd be glad for some advice with this. |
This needs to be implemented:
LWG-3897: inout_ptr will not update raw pointer to 0
https://cplusplus.github.io/LWG/issue3897
Maybe also static_assert Cpp17NullablePointer?