diff --git a/concat.md b/concat.md index d6ae8bf..293448b 100644 --- a/concat.md +++ b/concat.md @@ -358,7 +358,62 @@ went with the second constraint. In this paper, both are supported. `concat_view` can be `sized_range` if all the underlying ranges model `sized_range` -## Implementation experience +## `iter_swap` Customizations + +### Option 1: Delegate to the Underlying `iter_swap` + +This option was originally adopted in the paper. If all the combinations of the underlying iterators model `indirectly_swappable`, then `ranges::iter_swap(x, y)` could delegate to the underlying iterators + +```cpp +std::visit(ranges::iter_swap, x.it_, y.it_); +``` + +However, `ranges::iter_swap` is too permissive. Consider the following example: + +```cpp +int v1[] = {1, 2}; +long v2[] = {2147483649, 4}; +auto cv = std::views::concat(v1, v2); +auto it1 = cv.begin(); +auto it2 = cv.begin() + 2; +std::ranges::iter_swap(it1, it2); +``` + +Delegating `ranges::iter_swap(const concat_view::iterator&, const concat::iterator&)` to `ranges::iter_swap(int*, long*)` in this case would result in a tripple-move and leave `v1[0]` overflowed. + +Another example discussed in SG9 mailing is + +```cpp +std::string_view v1[] = {"aa"}; +std::string v2[] = {"bbb"}; +auto cv = std::views::concat(v1, v2); +auto it1 = cv.begin(); +auto it2 = cv.begin()+1; +std::ranges::iter_swap(it1, it2); +``` + +`ranges::iter_swap(string_view*, string*)` in this case would result in dangling reference due to the tripple move, which is effectively + +```cpp +void iter_swap_impl(string_view* x, string* y) +{ + std::string tmp(std::move(*y)); + *y = std::move(*x); + *x = std::move(tmp); // *x points to local string tmp +} +``` + +### Option 2: Do not Provide Customizations + +The above examples are no longer an issue because `ranges::iter_swap` would be ill-formed. + +However, all the user `iter_swap` customizations will be ignored. + +### Option 3: Tomasz's suggestion + + + +## Implementation Experience `views::concat` has been implemented in [@rangev3], with equivalent semantics as proposed here. The authors also implemented a version that directly follows the diff --git a/impl/concat/concat.hpp b/impl/concat/concat.hpp index 4573994..cdfdf76 100644 --- a/impl/concat/concat.hpp +++ b/impl/concat/concat.hpp @@ -70,16 +70,17 @@ consteval bool all_but_last(std::index_sequence) { } // namespace not_to_spec template -concept concat_is_random_access = ((random_access_range<__maybe_const> && - sized_range<__maybe_const>) && ...); +concept concat_is_random_access = ((random_access_range<__maybe_const> && + sized_range<__maybe_const>)&&...); template -concept constant_time_reversible = (bidirectional_range && common_range) || - (sized_range && random_access_range); +concept constant_time_reversible = + (bidirectional_range && common_range) || (sized_range && random_access_range); template -concept concat_bidirectional = all_but_last...>( - index_sequence_for{}) && bidirectional_range>; +concept concat_bidirectional = + all_but_last...>(index_sequence_for{}) && + bidirectional_range>; static_assert(true); // clang-format badness @@ -88,9 +89,7 @@ inline namespace not_to_spec { // it is not defined in the standard and we can't refer to it. template -concept has_member_arrow = requires(T it) { - it.operator->(); -}; +concept has_member_arrow = requires(T it) { it.operator->(); }; // iterator_traits::pointer when present gives the result of arrow, or presumably the result // of arrow is convertible to that. @@ -114,7 +113,7 @@ struct PointerTrait { }; template -requires requires { typename iterator_traits>::pointer; } + requires requires { typename iterator_traits>::pointer; } struct PointerTrait { using type = typename iterator_traits>::pointer; }; @@ -217,7 +216,7 @@ consteval auto iter_cat_base_sel() -> empty_; template consteval auto iter_cat_base_sel() -> iter_cat_base -requires((forward_range<__maybe_const> && ...)); + requires((forward_range<__maybe_const> && ...)); template using iter_cat_base_t = decltype(iter_cat_base_sel()); @@ -288,7 +287,8 @@ class concat_view : public view_interface> { template constexpr void advance_fwd(difference_type current_offset, difference_type steps) { - using underlying_diff_type = std::iter_difference_t>; + using underlying_diff_type = + std::iter_difference_t>; if constexpr (N == sizeof...(Views) - 1) { get(it_) += static_cast(steps); } else { @@ -297,14 +297,16 @@ class concat_view : public view_interface> { get(it_) += static_cast(steps); } else { it_.template emplace(ranges::begin(get(parent_->views_))); - advance_fwd(0, current_offset + steps - static_cast(n_size)); + advance_fwd( + 0, current_offset + steps - static_cast(n_size)); } } } template constexpr void advance_bwd(difference_type current_offset, difference_type steps) { - using underlying_diff_type = std::iter_difference_t>; + using underlying_diff_type = + std::iter_difference_t>; if constexpr (N == 0) { get(it_) -= static_cast(steps); } else { @@ -312,10 +314,10 @@ class concat_view : public view_interface> { get(it_) -= static_cast(steps); } else { auto prev_size = ranges::distance(get(parent_->views_)); - it_.template emplace(ranges::begin(get(parent_->views_)) + prev_size); - advance_bwd( - static_cast(prev_size), - steps - current_offset); + it_.template emplace(ranges::begin(get(parent_->views_)) + + prev_size); + advance_bwd(static_cast(prev_size), + steps - current_offset); } } } @@ -323,17 +325,18 @@ class concat_view : public view_interface> { decltype(auto) get_parent_views() const { return (parent_->views_); } template - explicit constexpr iterator(ParentView* parent, - Args&&... args) requires constructible_from - : parent_{parent}, it_{static_cast(args)...} {} + explicit constexpr iterator(ParentView* parent, Args&&... args) + requires constructible_from + : parent_{parent} + , it_{static_cast(args)...} {} public: iterator() = default; - constexpr iterator(iterator i) requires Const && - (convertible_to, iterator_t> && ...) + constexpr iterator(iterator i) + requires Const && (convertible_to, iterator_t> && ...) // [TODO] noexcept specs? : parent_{i.parent_} , it_{std::move(i.it_)} {} @@ -343,8 +346,9 @@ class concat_view : public view_interface> { return visit([](auto&& it) -> reference { return *it; }, it_); } - constexpr auto operator->() - const requires xo::concat_has_arrow<__maybe_const...> { + constexpr auto operator->() const + requires xo::concat_has_arrow<__maybe_const...> + { return visit( [](auto const& it) -> xo::concat_pointer_t<__maybe_const...> { using It = remove_reference_t; @@ -368,53 +372,64 @@ class concat_view : public view_interface> { constexpr void operator++(int) { ++*this; } - constexpr iterator operator++(int) requires( - forward_range<__maybe_const>&&...) { + constexpr iterator operator++(int) + requires(forward_range<__maybe_const> && ...) + { auto tmp = *this; ++*this; return tmp; } - constexpr iterator& operator--() requires - xo::concat_bidirectional<__maybe_const...> { + constexpr iterator& operator--() + requires xo::concat_bidirectional<__maybe_const...> + { xo::visit_i(it_, [this](auto I, auto&&) { this->prev(); }); return *this; } - constexpr iterator operator--( - int) requires xo::concat_bidirectional<__maybe_const...> { + constexpr iterator operator--(int) + requires xo::concat_bidirectional<__maybe_const...> + { auto tmp = *this; --*this; return tmp; } constexpr iterator& operator+=(difference_type n) // - requires xo::concat_is_random_access { + requires xo::concat_is_random_access + { if (n > 0) { xo::visit_i(it_, [this, n](auto I, auto&& it) { - this->advance_fwd(static_cast(it - ranges::begin(get(parent_->views_))), n); + this->advance_fwd( + static_cast(it - ranges::begin(get(parent_->views_))), + n); }); } else if (n < 0) { xo::visit_i(it_, [this, n](auto I, auto&& it) { - this->advance_bwd(static_cast(it - ranges::begin(get(parent_->views_))), -n); + this->advance_bwd( + static_cast(it - ranges::begin(get(parent_->views_))), + -n); }); } return *this; } constexpr iterator& operator-=(difference_type n) // - requires xo::concat_is_random_access { + requires xo::concat_is_random_access + { *this += -n; return *this; } constexpr decltype(auto) operator[](difference_type n) const // - requires xo::concat_is_random_access { + requires xo::concat_is_random_access + { return *((*this) + n); } - friend constexpr bool operator==(const iterator& it1, const iterator& it2) requires( - equality_comparable>>&&...) { + friend constexpr bool operator==(const iterator& it1, const iterator& it2) + requires(equality_comparable>> && ...) + { return it1.it_ == it2.it_; } @@ -424,49 +439,59 @@ class concat_view : public view_interface> { get(it.it_) == ranges::end(get(it.get_parent_views())); } - friend constexpr bool operator<(const iterator& x, const iterator& y) requires( - random_access_range<__maybe_const>&&...) { + friend constexpr bool operator<(const iterator& x, const iterator& y) + requires(random_access_range<__maybe_const> && ...) + { return x.it_ < y.it_; } - friend constexpr bool operator>(const iterator& x, const iterator& y) requires( - random_access_range<__maybe_const>&&...) { + friend constexpr bool operator>(const iterator& x, const iterator& y) + requires(random_access_range<__maybe_const> && ...) + { return y < x; } - friend constexpr bool operator<=(const iterator& x, const iterator& y) requires( - random_access_range<__maybe_const>&&...) { + friend constexpr bool operator<=(const iterator& x, const iterator& y) + requires(random_access_range<__maybe_const> && ...) + { return !(y < x); } - friend constexpr bool operator>=(const iterator& x, const iterator& y) requires( - random_access_range<__maybe_const>&&...) { + friend constexpr bool operator>=(const iterator& x, const iterator& y) + requires(random_access_range<__maybe_const> && ...) + { return !(x < y); } - friend constexpr auto operator<=>(const iterator& x, const iterator& y) requires( - (random_access_range<__maybe_const> && - three_way_comparable<__maybe_const>)&&...) { + friend constexpr auto operator<=>(const iterator& x, const iterator& y) + requires((random_access_range<__maybe_const> && + three_way_comparable<__maybe_const>) && + ...) + { return x.it_ <=> y.it_; } - friend constexpr iterator operator+(const iterator& it, difference_type n) requires - xo::concat_is_random_access { + friend constexpr iterator operator+(const iterator& it, difference_type n) + requires xo::concat_is_random_access + { return iterator{it} += n; } - friend constexpr iterator operator+(difference_type n, const iterator& it) requires - xo::concat_is_random_access { + friend constexpr iterator operator+(difference_type n, const iterator& it) + requires xo::concat_is_random_access + { return it + n; } - friend constexpr iterator operator-(const iterator& it, difference_type n) requires - xo::concat_is_random_access { + friend constexpr iterator operator-(const iterator& it, difference_type n) + requires xo::concat_is_random_access + { return iterator{it} -= n; } - friend constexpr difference_type operator-(const iterator& x, const iterator& y) requires - xo::concat_is_random_access { + friend constexpr difference_type operator-(const iterator& x, const iterator& y) + requires xo::concat_is_random_access + { auto ix = x.it_.index(); auto iy = y.it_.index(); if (ix > iy) { @@ -480,11 +505,13 @@ class concat_view : public view_interface> { difference_type(0)); auto y_to_end = xo::visit_i(y.it_, [&](auto I, auto&& it) { - return all_sizes[I] - (static_cast(it - ranges::begin(get(y.get_parent_views())))); + return all_sizes[I] - (static_cast( + it - ranges::begin(get(y.get_parent_views())))); }); auto begin_to_x = xo::visit_i(x.it_, [&](auto I, auto&& it) { - return static_cast(it - ranges::begin(get(x.get_parent_views()))); + return static_cast( + it - ranges::begin(get(x.get_parent_views()))); }); return y_to_end + in_between + begin_to_x; @@ -492,13 +519,15 @@ class concat_view : public view_interface> { } else if (ix < iy) { return -(y - x); } else { - return xo::visit_i(x.it_, - [&](auto I, auto&&) { return static_cast(get(x.it_) - get(y.it_)); }); + return xo::visit_i(x.it_, [&](auto I, auto&&) { + return static_cast(get(x.it_) - get(y.it_)); + }); } } - friend constexpr difference_type operator-(const iterator& it, default_sentinel_t) requires - xo::concat_is_random_access { + friend constexpr difference_type operator-(const iterator& it, default_sentinel_t) + requires xo::concat_is_random_access + { const auto idx = it.it_.index(); const auto all_sizes = std::apply( @@ -510,13 +539,15 @@ class concat_view : public view_interface> { std::accumulate(all_sizes.begin() + idx + 1, all_sizes.end(), difference_type(0)); auto i_to_idx_end = xo::visit_i(it.it_, [&](auto I, auto&& i) { - return all_sizes[I] - static_cast(i - ranges::begin(get(it.get_parent_views()))); + return all_sizes[I] - static_cast( + i - ranges::begin(get(it.get_parent_views()))); }); return -(i_to_idx_end + to_the_end); } - friend constexpr difference_type operator-(default_sentinel_t, const iterator& it) requires - xo::concat_is_random_access { + friend constexpr difference_type operator-(default_sentinel_t, const iterator& it) + requires xo::concat_is_random_access + { return -(it - default_sentinel); } @@ -526,20 +557,29 @@ class concat_view : public view_interface> { std::is_nothrow_convertible_v>, common_reference_t>...>>)&&...)) { - using common_r_value_ref_t = - common_reference_t>...>; return std::visit( - [](auto const& i) -> common_r_value_ref_t { // + [](auto const& i) -> xo::concat_rvalue_reference_t<__maybe_const...> { // return ranges::iter_move(i); }, ii.it_); } - friend constexpr void iter_swap(const iterator& x, const iterator& y) requires - requires(const iterator& a, const iterator& b) { - std::visit(ranges::iter_swap, x.it_, y.it_); + friend constexpr void iter_swap(const iterator& x, const iterator& y) + requires(indirectly_swappable>> && ... && + swappable_with...>, + xo::concat_reference_t<__maybe_const...>>) + // todo: noexcept? + { + std::visit( + [&](const auto& it1, const auto& it2) { + if constexpr (std::is_same_v) { + ranges::iter_swap(it1, it2); + } else { + ranges::swap(*x, *y); + } + }, + x.it_, y.it_); } - { std::visit(ranges::iter_swap, x.it_, y.it_); } }; @@ -549,7 +589,8 @@ class concat_view : public view_interface> { constexpr explicit concat_view(Views... views) : views_{static_cast(views)...} {} - constexpr iterator begin() requires(!(__simple_view && ...)) // + constexpr iterator begin() + requires(!(__simple_view && ...)) // { iterator it{this, in_place_index<0u>, ranges::begin(get<0>(views_))}; it.template satisfy<0>(); @@ -564,7 +605,9 @@ class concat_view : public view_interface> { return it; } - constexpr auto end() requires(!(__simple_view && ...)) { + constexpr auto end() + requires(!(__simple_view && ...)) + { using LastView = xo::back; if constexpr (common_range) { constexpr auto N = sizeof...(Views); @@ -574,7 +617,9 @@ class concat_view : public view_interface> { } } - constexpr auto end() const requires(range&&...) { + constexpr auto end() const + requires(range && ...) + { using LastView = xo::back; if constexpr (common_range) { constexpr auto N = sizeof...(Views); @@ -584,7 +629,9 @@ class concat_view : public view_interface> { } } - constexpr auto size() requires(sized_range&&...) { + constexpr auto size() + requires(sized_range && ...) + { return apply( [](auto... sizes) { using CT = make_unsigned_t>; @@ -593,7 +640,9 @@ class concat_view : public view_interface> { concat_detail::tuple_transform(ranges::size, views_)); } - constexpr auto size() const requires(sized_range&&...) { + constexpr auto size() const + requires(sized_range && ...) + { return apply( [](auto... sizes) { using CT = make_unsigned_t>; @@ -622,10 +671,10 @@ class concat_fn { } template - requires(sizeof...(V) > 1) && ranges::xo::concatable...> && - (viewable_range && ...) // - constexpr auto - operator()(V&&... v) const { // noexcept(noexcept(concat_view{static_cast(v)...})) { + requires(sizeof...(V) > 1) && ranges::xo::concatable...> && + (viewable_range && ...) // + constexpr auto operator()( + V&&... v) const { // noexcept(noexcept(concat_view{static_cast(v)...})) { return concat_view{static_cast(v)...}; } }; diff --git a/impl/concat/test/itermoveswap.cpp b/impl/concat/test/itermoveswap.cpp index 6a0f927..4ceb7d0 100644 --- a/impl/concat/test/itermoveswap.cpp +++ b/impl/concat/test/itermoveswap.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -103,10 +104,9 @@ struct throw_on_move { TEST_POINT("iter_move_throw_on_converting_to_rvalue_reference") { std::vector v; - auto tv = - std::views::iota(0) | std::views::transform([](auto) { return throw_on_move{}; }); - - auto cv = std::views::concat(v,tv); + auto tv = std::views::iota(0) | std::views::transform([](auto) { return throw_on_move{}; }); + + auto cv = std::views::concat(v, tv); using vector_rvalue_ref = std::ranges::range_rvalue_reference_t; using common_rvalue_ref = std::ranges::range_rvalue_reference_t; @@ -116,7 +116,6 @@ TEST_POINT("iter_move_throw_on_converting_to_rvalue_reference") { static_assert(std::same_as); static_assert(std::same_as); static_assert(!std::is_nothrow_convertible_v); - } TEST_POINT("iter_swap_concept") { @@ -193,4 +192,115 @@ TEST_POINT("largesort") { CHECK(vp == vp_expected); } -#endif \ No newline at end of file +#endif + +TEST_POINT("iter_swap_non_swappable_check1") { + int v1[] = {1, 2}; + long v2[] = {3, 4}; + auto cv = std::views::concat(v1, v2); + auto it1 = cv.begin(); + using iter = decltype(it1); + static_assert(!std::indirectly_swappable); +} + +TEST_POINT("iter_swap_non_swappable_check2") { + std::string_view v1[] = {"aa"}; + std::string v2[] = {"bbb"}; + auto cv = std::views::concat(v1, v2); + auto it1 = cv.begin(); + using iter = decltype(it1); + static_assert(!std::indirectly_swappable); +} + +struct iter_swap_iter { + int* i = nullptr; + int& operator*() const { return *i; } + + iter_swap_iter& operator++() { + ++i; + return *this; + } + iter_swap_iter operator++(int) { return {i + 1}; } + + bool operator==(iter_swap_iter other) const { return i == other.i; } + + using value_type = int; // to model indirectly_readable_traits + using difference_type = std::ptrdiff_t; // to model incrementable_traits + + friend void iter_swap(iter_swap_iter x, iter_swap_iter y) { + std::swap(*(x.i), *(y.i)); + *(x.i) += 10; + *(y.i) += 10; + } +}; + +struct iter_swap_iter_range { + std::vector vec; + iter_swap_iter begin() { return iter_swap_iter{vec.data()}; } + iter_swap_iter end() { return iter_swap_iter{vec.data() + vec.size()}; } +}; + +TEST_POINT("iter_swap_customisation") { + iter_swap_iter_range r1; + r1.vec = {1, 2, 3}; + std::vector r2 = {4, 5, 6}; + auto cv = std::views::concat(r1, r2); + + // same range with customisation + auto it1 = cv.begin(); + auto it2 = cv.begin(); + ++it2; + std::ranges::iter_swap(it1, it2); + CHECK(r1.vec[0] == 12); + CHECK(r1.vec[1] == 11); + + auto it3 = cv.begin(); + ++it3; + ++it3; + + auto it4 = it3; + ++it4; + + std::ranges::iter_swap(it3, it4); + CHECK(r1.vec[2] == 4); + CHECK(r2[0] == 3); +} + + +TEST_POINT("tricky_tuple_case") { + std::vector> r1{ + {1, 2, 3}, + {4, 5, 6}, + }; + + std::vector v1{ + 7, + 8, + }; + + std::vector v2{ + 9, + 10, + }; + + std::vector v3{ + 11, + 12, + }; + + auto r2 = std::views::zip(v1, v2, v3); + // {7, 9, 11} + // {8, 10, 12} + + auto cv = std::views::concat(r1, r2); + + auto it1 = cv.begin(); + auto it2 = cv.begin() + 2; + std::ranges::iter_swap(it1, it2); + // swap (1,2,3) with (7,9,11) + + CHECK(r1[0] == std::tuple(7, 9, 11)); + CHECK(v1[0] == 1); + CHECK(v2[0] == 2); + CHECK(v3[0] == 3); +}