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

Make uninitialized_meow helpers constexpr #1329

Closed
wants to merge 12 commits into from

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Sep 30, 2020

This add some preparatory work to make our internal _Copy_meow and _Move_meow helper functions constexpr enabled.

I believe that some of those changes are needed for correctness during constexpr as we need to actually move if we directly emply the helpers without checking for trivial types (looking at vector)

As far as I understand we currently do not support the allocator siblings of those functions

Partially addresses #45

@miscco miscco requested a review from a team as a code owner September 30, 2020 19:30
@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Sep 30, 2020
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated
@@ -1682,7 +1723,7 @@ _Alloc_ptr_t<_Alloc> _Uninitialized_move(
const auto _ULast = _Get_unwrapped(_Last);
if constexpr (conjunction_v<bool_constant<_Ptr_move_cat<decltype(_UFirst), _Ptrval>::_Really_trivial>,
_Uses_default_construct<_Alloc, _Ptrval, decltype(_STD move(*_UFirst))>>) {
_Copy_memmove(_UFirst, _ULast, _Unfancy(_Dest));
_Move_memmove(_UFirst, _ULast, _Unfancy(_Dest));
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 think we need to wait until we have the contexpr allocator_traits<_Alloc>::construct

const same_as<int_wrapper_move*> auto result = _Move_memmove(begin(input), end(input), begin(output));
assert(result == end(output));
assert(equal(begin(expected_move), end(expected_move), begin(output), end(output)));
if (is_constant_evaluated()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these functions currently require that we have trivial types.

To feed my paranoia and as I expect them to be used within containers I added those tests

stl/inc/xutility Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Oct 1, 2020

@CaseyCarter I am currently breaking ranges::move_backward because I forgot to update it too.

I think just changing the function is correct, however it is a bit ugly. We would have to use iter_move for the ranges versions.

We might want to think whether this splitting up is really the right way forward.

@miscco miscco force-pushed the constexpr_uninitialized_meow branch 2 times, most recently from d4f824c to 5ff86d1 Compare October 1, 2020 12:27
@miscco
Copy link
Contributor Author

miscco commented Oct 5, 2020

Having looked at this a bit more I decided that I do not like the duplication/indirection at all and will come up with a different version soonish

@miscco miscco force-pushed the constexpr_uninitialized_meow branch 3 times, most recently from c36141c to 3943390 Compare October 7, 2020 10:10
@CaseyCarter

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@cbezault
Copy link
Contributor

cbezault commented Oct 7, 2020

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@@ -4559,11 +4567,6 @@ _BidIt2 _Copy_backward_memmove(_BidIt1 _First, _BidIt1 _Last, _BidIt2 _Dest) {
return static_cast<_BidIt2>(_CSTD memmove(_Dest_ch - _Count, _First_ch, _Count));
}

template <class _BidIt1, class _BidIt2>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and its forward friend are the only things I could imagine to break something

@miscco miscco force-pushed the constexpr_uninitialized_meow branch from d2b1d42 to a3e3dc8 Compare October 13, 2020 08:54
@miscco miscco changed the title Implement constexpr versions of the meow_memmove helpers Make uninitialized_meow helpers constexpr Oct 13, 2020
@miscco
Copy link
Contributor Author

miscco commented Oct 13, 2020

@mnatsuhara @CaseyCarter I rebased this on top of #872, which enabled me to find the bug I introduced.

So this PR is essentially only the final commit.

@CaseyCarter CaseyCarter self-assigned this Oct 15, 2020
@miscco
Copy link
Contributor Author

miscco commented Oct 29, 2020

Dropping this in favor of #1407, where it actually applies

@miscco miscco closed this Oct 29, 2020
@miscco miscco deleted the constexpr_uninitialized_meow branch October 29, 2020 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants