From a2b9f6db336c4dd3022d1cffbcdddcd36fad530d Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Fri, 17 Jul 2020 11:07:48 -0700 Subject: [PATCH 1/7] Implement ranges::move_backward --- stl/inc/algorithm | 46 +++++++++ tests/std/test.lst | 1 + .../P0896R4_ranges_alg_move_backward/env.lst | 4 + .../P0896R4_ranges_alg_move_backward/test.cpp | 94 +++++++++++++++++++ 4 files changed, 145 insertions(+) create mode 100644 tests/std/tests/P0896R4_ranges_alg_move_backward/env.lst create mode 100644 tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp diff --git a/stl/inc/algorithm b/stl/inc/algorithm index 80a0ce54552..d81a361bddb 100644 --- a/stl/inc/algorithm +++ b/stl/inc/algorithm @@ -1789,6 +1789,52 @@ namespace ranges { }; inline constexpr _Move_fn move{_Not_quite_object::_Construct_tag{}}; + + // ALIAS TEMPLATE move_backward_result + template + using move_backward_result = in_out_result<_In, _Out>; + + // VARIABLE ranges::move_backward + class _Move_backward_fn : private _Not_quite_object { + public: + using _Not_quite_object::_Not_quite_object; + + // clang-format off + template _Se1, bidirectional_iterator _It2> + requires indirectly_movable<_It1, _It2> + constexpr move_backward_result<_It1, _It2> operator()(_It1 _First, _Se1 _Last, _It2 _Result) const { + _Adl_verify_range(_First, _Last); + auto _UFirst = _Get_unwrapped(_STD move(_First)); + auto _ULast = _Get_final_iterator_unwrapped<_It1>(_UFirst, _STD move(_Last)); + _Seek_wrapped(_First, _ULast); + _Result = _Move_backward_common(_STD move(_UFirst), _STD move(_ULast), _STD move(_Result)); + return {_STD move(_First), _STD move(_Result)}; + } + + template + requires indirectly_movable, _It> + constexpr move_backward_result, _It> operator()(_Rng&& _Range, _It _Result) const { + auto _ULast = _Get_final_iterator_unwrapped(_Range); + _Result = _Move_backward_common(_Ubegin(_Range), _ULast, _STD move(_Result)); + return {_Rewrap_iterator(_Range, _STD move(_ULast)), _STD move(_Result)}; + } + // clang-format on + private: + template + _NODISCARD static constexpr _It2 _Move_backward_common(const _It1 _First, _It1 _Last, _It2 _Result) { + _STL_INTERNAL_STATIC_ASSERT(bidirectional_iterator<_It1>); + _STL_INTERNAL_STATIC_ASSERT(bidirectional_iterator<_It2>); + _STL_INTERNAL_STATIC_ASSERT(indirectly_movable<_It1, _It2>); + + while (_First != _Last) { + *--_Result = _RANGES iter_move(--_Last); + } + + return _Result; + } + }; + + inline constexpr _Move_backward_fn move_backward{_Not_quite_object::_Construct_tag{}}; } // namespace ranges #endif // __cpp_lib_concepts diff --git a/tests/std/test.lst b/tests/std/test.lst index 56b6f4f7191..b098195b521 100644 --- a/tests/std/test.lst +++ b/tests/std/test.lst @@ -264,6 +264,7 @@ tests\P0896R4_ranges_alg_is_sorted tests\P0896R4_ranges_alg_minmax tests\P0896R4_ranges_alg_mismatch tests\P0896R4_ranges_alg_move +tests\P0896R4_ranges_alg_move_backward tests\P0896R4_ranges_alg_none_of tests\P0896R4_ranges_alg_partition tests\P0896R4_ranges_alg_partition_copy diff --git a/tests/std/tests/P0896R4_ranges_alg_move_backward/env.lst b/tests/std/tests/P0896R4_ranges_alg_move_backward/env.lst new file mode 100644 index 00000000000..f3ccc8613c6 --- /dev/null +++ b/tests/std/tests/P0896R4_ranges_alg_move_backward/env.lst @@ -0,0 +1,4 @@ +# Copyright (c) Microsoft Corporation. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +RUNALL_INCLUDE ..\concepts_matrix.lst diff --git a/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp new file mode 100644 index 00000000000..c211472f22b --- /dev/null +++ b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp @@ -0,0 +1,94 @@ +// Copyright (c) Microsoft Corporation. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include +#include +#include +#include +#include + +#include + +using namespace std; + +struct int_wrapper { + int val = 10; + constexpr int_wrapper() = default; + constexpr int_wrapper(int x) : val{x} {} + constexpr int_wrapper(int_wrapper&& that) : val{exchange(that.val, -1)} {} + constexpr int_wrapper& operator=(int_wrapper&& that) { + val = exchange(that.val, -1); + return *this; + } + auto operator<=>(const int_wrapper&) const = default; +}; + +// Validate that move_backward_result aliases in_out_result +STATIC_ASSERT(same_as, ranges::in_out_result>); + +// Validate dangling story +STATIC_ASSERT(same_as{}, static_cast(nullptr))), + ranges::move_backward_result>); +STATIC_ASSERT(same_as{}, static_cast(nullptr))), + ranges::move_backward_result>); + +static constexpr int expected_output[] = {13, 42, 1729}; +static constexpr int expected_input[] = {-1, -1, -1}; +static constexpr int expected_overlapping[] = {-1, 0, 1, 2}; + +struct instantiator { + template + static constexpr void call() { +#if !defined(__clang__) && !defined(__EDG__) // TRANSITION, VSO-938163 +#pragma warning(suppress : 4127) // conditional expression is constant + if (!ranges::contiguous_range || !ranges::contiguous_range || !is_constant_evaluated()) +#endif // TRANSITION, VSO-938163 + { + // For the second range, we need an iterator to the end; it's expedient to simply ignore ranges with + // differing iterator and sentinel types (i.e., ranges that don't satisfy common_range). + if constexpr (ranges::common_range) { + using ranges::move_backward, ranges::move_backward_result, ranges::equal, ranges::iterator_t; + + { // Validate range overload + int_wrapper input[] = {13, 42, 1729}; + int_wrapper output[] = {-2, -2, -2}; + R1 wrapped_input{input}; + R2 wrapped_output{output}; + same_as, iterator_t>> auto result = + move_backward(wrapped_input, wrapped_output.end()); + assert(result.in == wrapped_input.end()); + assert(result.out == wrapped_output.begin()); + assert(equal(output, expected_output, ranges::equal_to{}, &int_wrapper::val)); + assert(equal(input, expected_input, ranges::equal_to{}, &int_wrapper::val)); + } + { // Validate iterator + sentinel overload + int_wrapper input[] = {13, 42, 1729}; + int_wrapper output[] = {-2, -2, -2}; + R1 wrapped_input{input}; + R2 wrapped_output{output}; + same_as, iterator_t>> auto result = + move_backward(wrapped_input.begin(), wrapped_input.end(), wrapped_output.end()); + assert(result.in == wrapped_input.end()); + assert(result.out == wrapped_output.begin()); + assert(equal(output, expected_output, ranges::equal_to{}, &int_wrapper::val)); + assert(equal(input, expected_input, ranges::equal_to{}, &int_wrapper::val)); + } + { // Validate overlapping ranges + int_wrapper io[] = {0, 1, 2, 42}; + R1 wrapped_input{span{io}.first<3>()}; + R2 wrapped_output{span{io}.last<3>()}; + same_as, iterator_t>> auto result = + move_backward(wrapped_input, wrapped_output.end()); + assert(result.in == wrapped_input.end()); + assert(result.out == wrapped_output.begin()); + assert(equal(io, expected_overlapping, ranges::equal_to{}, &int_wrapper::val)); + } + } + } + } +}; + +int main() { + STATIC_ASSERT((test_bidi_bidi(), true)); + test_bidi_bidi(); +} From 0af3e4ee3de68becff444cbdba6cd81ba69d194f Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Fri, 17 Jul 2020 18:34:50 -0700 Subject: [PATCH 2/7] Review comments --- tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp index c211472f22b..19a09cca1ed 100644 --- a/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp +++ b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp @@ -6,6 +6,8 @@ #include #include #include +#include +#include #include @@ -27,9 +29,9 @@ struct int_wrapper { STATIC_ASSERT(same_as, ranges::in_out_result>); // Validate dangling story -STATIC_ASSERT(same_as{}, static_cast(nullptr))), +STATIC_ASSERT(same_as{}, nullptr_to)), ranges::move_backward_result>); -STATIC_ASSERT(same_as{}, static_cast(nullptr))), +STATIC_ASSERT(same_as{}, nullptr_to)), ranges::move_backward_result>); static constexpr int expected_output[] = {13, 42, 1729}; @@ -41,7 +43,7 @@ struct instantiator { static constexpr void call() { #if !defined(__clang__) && !defined(__EDG__) // TRANSITION, VSO-938163 #pragma warning(suppress : 4127) // conditional expression is constant - if (!ranges::contiguous_range || !ranges::contiguous_range || !is_constant_evaluated()) + if (!ranges::contiguous_range || !is_constant_evaluated()) #endif // TRANSITION, VSO-938163 { // For the second range, we need an iterator to the end; it's expedient to simply ignore ranges with From 6abb1a0e789570a5622c9e3b6b372135e602a3c1 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Sun, 19 Jul 2020 05:58:22 -0700 Subject: [PATCH 3/7] miscco review comments --- stl/inc/algorithm | 6 ++ .../P0896R4_ranges_alg_move_backward/test.cpp | 60 ++++++++++++++++--- 2 files changed, 59 insertions(+), 7 deletions(-) diff --git a/stl/inc/algorithm b/stl/inc/algorithm index d81a361bddb..19dd420b9eb 100644 --- a/stl/inc/algorithm +++ b/stl/inc/algorithm @@ -1826,6 +1826,12 @@ namespace ranges { _STL_INTERNAL_STATIC_ASSERT(bidirectional_iterator<_It2>); _STL_INTERNAL_STATIC_ASSERT(indirectly_movable<_It1, _It2>); + if constexpr (_Ptr_move_cat<_It1, _It2>::_Trivially_copyable) { + if (!_STD is_constant_evaluated()) { + return _Copy_backward_memmove(_First, _Last, _Result); + } + } + while (_First != _Last) { *--_Result = _RANGES iter_move(--_Last); } diff --git a/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp index 19a09cca1ed..b0e4a4a9c52 100644 --- a/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp +++ b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp @@ -14,7 +14,8 @@ using namespace std; struct int_wrapper { - int val = 10; + int val = 10; + constexpr int_wrapper() = default; constexpr int_wrapper(int x) : val{x} {} constexpr int_wrapper(int_wrapper&& that) : val{exchange(that.val, -1)} {} @@ -34,11 +35,11 @@ STATIC_ASSERT(same_as{}, nullptr_ STATIC_ASSERT(same_as{}, nullptr_to)), ranges::move_backward_result>); -static constexpr int expected_output[] = {13, 42, 1729}; -static constexpr int expected_input[] = {-1, -1, -1}; -static constexpr int expected_overlapping[] = {-1, 0, 1, 2}; - struct instantiator { + static constexpr int expected_output[] = {13, 42, 1729}; + static constexpr int expected_input[] = {-1, -1, -1}; + static constexpr int expected_overlapping[] = {-1, 0, 1, 2}; + template static constexpr void call() { #if !defined(__clang__) && !defined(__EDG__) // TRANSITION, VSO-938163 @@ -90,7 +91,52 @@ struct instantiator { } }; +constexpr void test_memmove() { + // Get some coverage for the memmove optimization, which we would not otherwise have since we do not currently + // unwrap output iterators. TRANSITION, GH-893 + using ranges::move_backward, ranges::move_backward_result, ranges::begin, ranges::end, ranges::equal; + + struct S { // move-only and trivially copyable + int val = 10; + + constexpr S() = default; + constexpr S(int x) : val{x} {} + constexpr S(S&&) = default; + constexpr S& operator=(S&&) = default; + auto operator<=>(const S&) const = default; + }; + + + { // Validate range overload + S input[] = {13, 42, 1729}; + S output[] = {-2, -2, -2}; + const same_as> auto result = move_backward(input, end(output)); + assert(result.in == end(input)); + assert(result.out == begin(output)); + assert(equal(output, input)); + } + { // Validate iterator + sentinel overload + S input[] = {13, 42, 1729}; + S output[] = {-2, -2, -2}; + const same_as> auto result = move_backward(begin(input), end(input), end(output)); + assert(result.in == end(input)); + assert(result.out == begin(output)); + assert(equal(output, input)); + } + { // Validate overlapping ranges + S io[] = {0, 1, 2, 42}; + const same_as> auto result = move_backward(io + 0, io + 3, io + 4); + assert(result.in == io + 3); + assert(result.out == io + 1); + const int expected[] = {0, 0, 1, 2}; + assert(equal(io, expected, ranges::equal_to{}, &S::val)); + } +} + int main() { - STATIC_ASSERT((test_bidi_bidi(), true)); - test_bidi_bidi(); + // FIXME: STATIC_ASSERT((test_bidi_bidi(), true)); + // FIXME: test_bidi_bidi(); + + STATIC_ASSERT((test_memmove(), true)); + test_memmove(); } From 4c82da14a46669bff490de944ebe098d429d3b1f Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Sun, 19 Jul 2020 06:01:46 -0700 Subject: [PATCH 4/7] Maybe it's better to run the tests than comment them out --- tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp index b0e4a4a9c52..e682edaad93 100644 --- a/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp +++ b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp @@ -134,8 +134,8 @@ constexpr void test_memmove() { } int main() { - // FIXME: STATIC_ASSERT((test_bidi_bidi(), true)); - // FIXME: test_bidi_bidi(); + STATIC_ASSERT((test_bidi_bidi(), true)); + test_bidi_bidi(); STATIC_ASSERT((test_memmove(), true)); test_memmove(); From a75ccf96d933fcd15bed8c282869f21a03198a71 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Sun, 19 Jul 2020 08:35:16 -0700 Subject: [PATCH 5/7] Promote _Move_backward_common to namespace scope (for at least inplace_merge) --- stl/inc/algorithm | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/stl/inc/algorithm b/stl/inc/algorithm index 19dd420b9eb..92838eca06c 100644 --- a/stl/inc/algorithm +++ b/stl/inc/algorithm @@ -1795,6 +1795,25 @@ namespace ranges { using move_backward_result = in_out_result<_In, _Out>; // VARIABLE ranges::move_backward + // clang-format off + // concept-constrained for strict enforcement as it is used by several algorithms + template + requires indirectly_movable<_It1, _It2> + _NODISCARD static constexpr _It2 _Move_backward_common(const _It1 _First, _It1 _Last, _It2 _Result) { + if constexpr (_Ptr_move_cat<_It1, _It2>::_Trivially_copyable) { + if (!_STD is_constant_evaluated()) { + return _Copy_backward_memmove(_First, _Last, _Result); + } + } + + while (_First != _Last) { + *--_Result = _RANGES iter_move(--_Last); + } + + return _Result; + } + // clang-format on + class _Move_backward_fn : private _Not_quite_object { public: using _Not_quite_object::_Not_quite_object; @@ -1807,7 +1826,7 @@ namespace ranges { auto _UFirst = _Get_unwrapped(_STD move(_First)); auto _ULast = _Get_final_iterator_unwrapped<_It1>(_UFirst, _STD move(_Last)); _Seek_wrapped(_First, _ULast); - _Result = _Move_backward_common(_STD move(_UFirst), _STD move(_ULast), _STD move(_Result)); + _Result = _RANGES _Move_backward_common(_STD move(_UFirst), _STD move(_ULast), _STD move(_Result)); return {_STD move(_First), _STD move(_Result)}; } @@ -1815,29 +1834,10 @@ namespace ranges { requires indirectly_movable, _It> constexpr move_backward_result, _It> operator()(_Rng&& _Range, _It _Result) const { auto _ULast = _Get_final_iterator_unwrapped(_Range); - _Result = _Move_backward_common(_Ubegin(_Range), _ULast, _STD move(_Result)); + _Result = _RANGES _Move_backward_common(_Ubegin(_Range), _ULast, _STD move(_Result)); return {_Rewrap_iterator(_Range, _STD move(_ULast)), _STD move(_Result)}; } // clang-format on - private: - template - _NODISCARD static constexpr _It2 _Move_backward_common(const _It1 _First, _It1 _Last, _It2 _Result) { - _STL_INTERNAL_STATIC_ASSERT(bidirectional_iterator<_It1>); - _STL_INTERNAL_STATIC_ASSERT(bidirectional_iterator<_It2>); - _STL_INTERNAL_STATIC_ASSERT(indirectly_movable<_It1, _It2>); - - if constexpr (_Ptr_move_cat<_It1, _It2>::_Trivially_copyable) { - if (!_STD is_constant_evaluated()) { - return _Copy_backward_memmove(_First, _Last, _Result); - } - } - - while (_First != _Last) { - *--_Result = _RANGES iter_move(--_Last); - } - - return _Result; - } }; inline constexpr _Move_backward_fn move_backward{_Not_quite_object::_Construct_tag{}}; From 30a485914a50c81bef27188732ad637c44047bcc Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Tue, 21 Jul 2020 12:51:58 -0700 Subject: [PATCH 6/7] Apply suggestions from code review Co-authored-by: Stephan T. Lavavej --- stl/inc/algorithm | 2 +- tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/stl/inc/algorithm b/stl/inc/algorithm index 92838eca06c..0b1617a4407 100644 --- a/stl/inc/algorithm +++ b/stl/inc/algorithm @@ -1799,7 +1799,7 @@ namespace ranges { // concept-constrained for strict enforcement as it is used by several algorithms template requires indirectly_movable<_It1, _It2> - _NODISCARD static constexpr _It2 _Move_backward_common(const _It1 _First, _It1 _Last, _It2 _Result) { + _NODISCARD constexpr _It2 _Move_backward_common(const _It1 _First, _It1 _Last, _It2 _Result) { if constexpr (_Ptr_move_cat<_It1, _It2>::_Trivially_copyable) { if (!_STD is_constant_evaluated()) { return _Copy_backward_memmove(_First, _Last, _Result); diff --git a/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp index e682edaad93..239a286a31e 100644 --- a/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp +++ b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp @@ -128,7 +128,7 @@ constexpr void test_memmove() { const same_as> auto result = move_backward(io + 0, io + 3, io + 4); assert(result.in == io + 3); assert(result.out == io + 1); - const int expected[] = {0, 0, 1, 2}; + constexpr int expected[] = {0, 0, 1, 2}; assert(equal(io, expected, ranges::equal_to{}, &S::val)); } } From 23d68f2153066d8497bd5598c7211f3e259525c4 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Mon, 27 Jul 2020 13:00:45 -0700 Subject: [PATCH 7/7] Update tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp Co-authored-by: mnatsuhara <46756417+mnatsuhara@users.noreply.github.com> --- tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp index 239a286a31e..06c17f7953f 100644 --- a/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp +++ b/tests/std/tests/P0896R4_ranges_alg_move_backward/test.cpp @@ -106,7 +106,6 @@ constexpr void test_memmove() { auto operator<=>(const S&) const = default; }; - { // Validate range overload S input[] = {13, 42, 1729}; S output[] = {-2, -2, -2};