-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement P1132R7 out_ptr() and inout_ptr() #1998
Conversation
Awesome! I was wondering whether I should give it a try but you beat me to it. Will Review tomorrow |
stl/inc/memory
Outdated
|
||
operator void**() const noexcept requires(!is_same_v<_Pointer, void*>) { | ||
static_assert(is_pointer_v<_Pointer>, "out_ptr_t's operator void** requires Pointer to be a pointer"); | ||
return reinterpret_cast<void**>(_STD addressof(const_cast<_Pointer&>(_Ptr))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the precondition that we should not have called operator _Pointer*
Do we care enough to actually validate this somehow? It would most likely cost some memory so I am totally fine with nasal demons emerging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's worth validating. It will be used as foo(out_ptr(my_ptr))
99.9% of the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Adam. It's not so much the memory as it is having the ABI of out_ptr
differ between debug and release modes. We already do that with many types, but doing it here will probably cause more harm than good, given Adam's point about common usage
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add the feature test macros, additionally I found the error messages produced by the static_asserts to be possibly confusing, but I don't care all that strongly about that.
I observe that we may be able to apply certain optimizations under the "as if" rule to this type. In particular for raw and unique ptrs. Raw pointers maybe aren't worth it, but unique_ptrs might be.
I think the optimization involves calling release in the constructor and then not storing the reference to the smart pointer itself. I'm OK with not applying such an optimization but we should keep it in mind.
stl/inc/memory
Outdated
|
||
operator void**() const noexcept requires(!is_same_v<_Pointer, void*>) { | ||
static_assert(is_pointer_v<_Pointer>, "out_ptr_t's operator void** requires Pointer to be a pointer"); | ||
return reinterpret_cast<void**>(_STD addressof(const_cast<_Pointer&>(_Ptr))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Adam. It's not so much the memory as it is having the ABI of out_ptr
differ between debug and release modes. We already do that with many types, but doing it here will probably cause more harm than good, given Adam's point about common usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! My major question is whether it's worth compressing away empty tuple<>
s; everything else is nitpick-level.
@AdamBucior @barcharcraz I've pushed a merge with |
Thanks for implementing this C++23 feature! 😻 🚀 🎉 |
Resolves #1971.