Skip to content
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

Merged
merged 11 commits into from
Jul 30, 2021

Conversation

AdamBucior
Copy link
Contributor

Resolves #1971.

@AdamBucior AdamBucior requested a review from a team as a code owner June 10, 2021 15:48
stl/inc/memory Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor

miscco commented Jun 10, 2021

Awesome!

I was wondering whether I should give it a try but you beat me to it. Will Review tomorrow

stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the cxx23 C++23 feature label Jun 10, 2021
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
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)));
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Member

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

@StephanTLavavej

This comment has been minimized.

@StephanTLavavej

This comment has been minimized.

Copy link
Member

@barcharcraz barcharcraz left a 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/yvals_core.h Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
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)));
Copy link
Member

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

stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
Copy link
Member

@StephanTLavavej StephanTLavavej left a 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.

tests/std/tests/P1132R7_out_ptr/test.cpp Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
stl/inc/memory Outdated Show resolved Hide resolved
tests/std/tests/P1132R7_out_ptr/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P1132R7_out_ptr/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Jul 13, 2021
@StephanTLavavej StephanTLavavej removed their assignment Jul 14, 2021
@StephanTLavavej
Copy link
Member

@AdamBucior @barcharcraz I've pushed a merge with main, followed by a commit to avoid clang-format 12 damaging a couple of lines.

@StephanTLavavej StephanTLavavej self-assigned this Jul 29, 2021
@StephanTLavavej StephanTLavavej merged commit b24a40d into microsoft:main Jul 30, 2021
@StephanTLavavej
Copy link
Member

Thanks for implementing this C++23 feature! 😻 🚀 🎉

@AdamBucior AdamBucior deleted the out_ptr branch July 30, 2021 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P1132R7 out_ptr(), inout_ptr()
6 participants
  翻译: