From 3bbd18b1316e82ddad72aacf6513ba3bd583b5b0 Mon Sep 17 00:00:00 2001 From: Hui Date: Mon, 5 Jun 2023 17:14:42 +0100 Subject: [PATCH] `concat` difference_type fix --- concat.lwg.feedback.md | 8 ++++---- concat.md | 30 ++++++++++++++++-------------- impl/concat/concat.hpp | 32 +++++++++++++++++--------------- 3 files changed, 37 insertions(+), 33 deletions(-) diff --git a/concat.lwg.feedback.md b/concat.lwg.feedback.md index 8594794..9e2a55f 100644 --- a/concat.lwg.feedback.md +++ b/concat.lwg.feedback.md @@ -17,6 +17,10 @@ [Done] there is iterator const& which should be const iterator& (this is editorial) [Done] 2.1 should say is_reference_v [Done] all the other bullets need to start with "otherwise" +[Done] you should use ranges::distance and then you don't need them +[Done] instead of ranges::size this should be ranges::distance and we don't need the static_cast. +[Done] all the steps need to cast to that iterator's difference_type +[Done] al the steps need to be cast to range::diffence_type # Todos @@ -30,11 +34,7 @@ [TODO] the things which use the general concepts are replaced: op++, op<, etc. [TODO] (1.3) use all_forward [TODO] OK, we can keep it as is but I prefer < -[TODO] instead of ranges::size this should be ranges::distance and we don't need the static_cast -[TODO] all the steps need to cast to that iterator's difference_type [TODO] do we want to avoid emplacing ranges we don't need to use? -[TODO] you should use ranges::distance and then you don't need them -[TODO] al the steps need to be cast to range::diffence_type [TODO] explain why this is over-constrained [TODO] the use of maybe-const on the requirement of the constructor: I think we removed them elsewhere as we Const is true [TODO] for op*() p10 can use concat_reference_t diff --git a/concat.md b/concat.md index 49722fd..5754a1a 100644 --- a/concat.md +++ b/concat.md @@ -831,15 +831,16 @@ constexpr void @_advance-fwd_@(difference_type offset, difference_type steps); / [5]{.pnum} *Effects*: Equivalent to: ```cpp +using underlying_diff_type = iter_difference_t>; if constexpr (N == sizeof...(Views) - 1) { - get(@*it_*@) += steps; + get(@*it_*@) += static_cast(steps); } else { - auto n_size = ranges::size(get(@*parent_*@->@*views_*@)); + auto n_size = ranges::distance(get(@*parent_*@->@*views_*@)); if (offset + steps < static_cast(n_size)) { - get(@*it_*@) += steps; + get(@*it_*@) += static_cast(steps); } else { @*it_*@.template emplace(ranges::begin(get(@*parent_*@->@*views_*@))); - @*advance-fwd*@(0, offset + steps - n_size); + @*advance-fwd*@(0, offset + steps - static_cast(n_size)); } } ``` @@ -856,16 +857,17 @@ constexpr void @_advance-bwd_@(difference_type offset, difference_type steps); / [6]{.pnum} *Effects*: Equivalent to: ```cpp +using underlying_diff_type = iter_difference_t>; if constexpr (N == 0) { - get(@*it_*@) -= steps; + get(@*it_*@) -= static_cast(steps); } else { if (offset >= steps) { - get(@*it_*@) -= steps; + get(@*it_*@) -= static_cast(steps); } else { - @*it_*@.template emplace(ranges::begin(get(@*parent_*@->@*views_*@)) + - ranges::size(get(@*parent_*@->@*views_*@))); + auto prev_size = ranges::distance(get(@*parent_*@->@*views_*@)); + @*it_*@.template emplace(ranges::begin(get(@*parent_*@->@*views_*@)) + prev_size); @*advance-bwd*@( - static_cast(ranges::size(get(@*parent_*@->@*views_*@))), + static_cast(prev_size), steps - offset); } } @@ -1022,9 +1024,9 @@ constexpr @_iterator_@& operator+=(difference_type n) ```cpp if(n > 0) { - @*advance-fwd*@<@*i*@>(get<@*i*@>(@*it_*@) - ranges::begin(get<@*i*@>(@*parent_*@->@*views_*@)), n); + @*advance-fwd*@<@*i*@>(static_cast(@*it_*@) - ranges::begin(get<@*i*@>(@*parent_*@->@*views_*@))), n); } else if (n < 0) { - @*advance-bwd*@<@*i*@>(get<@*i*@>(@*it_*@) - ranges::begin(get<@*i*@>(@*parent_*@->@*views_*@)), -n); + @*advance-bwd*@<@*i*@>(static_cast(@*it_*@) - ranges::begin(get<@*i*@>(@*parent_*@->@*views_*@))), -n); } return *this; ``` @@ -1267,7 +1269,7 @@ denote `y.@*it_*@.index()` equivalent to ```cpp - return @*d~y~*@ + s + @*d~x~*@; + return static_cast(@*d~y~*@) + s + static_cast(@*d~x~*@); ``` - [48.2]{.pnum} otherwise, if `@*i~x~*@ < @*i~y~*@`, equivalent to: @@ -1279,7 +1281,7 @@ denote `y.@*it_*@.index()` - [48.3]{.pnum} otherwise, equivalent to: ```cpp - return get<@*i~x~*@>(x.@*it_*@) - get<@*i~y~*@>(y.@*it_*@); + return static_cast(get<@*i~x~*@>(x.@*it_*@) - get<@*i~y~*@>(y.@*it_*@)); ``` ::: @@ -1301,7 +1303,7 @@ all the ranges `get<@*i*@>(x.@*parent_*@.@*views_*@)` if there is any, and `0` otherwise, equivalent to ```cpp -return -(@*d~x~*@ + s); +return -(static_cast(@*d~x~*@) + static_cast(s)); ``` ::: diff --git a/impl/concat/concat.hpp b/impl/concat/concat.hpp index ef7b44c..34b3863 100644 --- a/impl/concat/concat.hpp +++ b/impl/concat/concat.hpp @@ -287,31 +287,33 @@ 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>; if constexpr (N == sizeof...(Views) - 1) { - get(it_) += steps; + get(it_) += static_cast(steps); } else { - auto n_size = ranges::size(get(parent_->views_)); + auto n_size = ranges::distance(get(parent_->views_)); if (current_offset + steps < static_cast(n_size)) { - get(it_) += steps; + get(it_) += static_cast(steps); } else { it_.template emplace(ranges::begin(get(parent_->views_))); - advance_fwd(0, current_offset + steps - 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>; if constexpr (N == 0) { - get(it_) -= steps; + get(it_) -= static_cast(steps); } else { if (current_offset >= steps) { - get(it_) -= steps; + get(it_) -= static_cast(steps); } else { - it_.template emplace(ranges::begin(get(parent_->views_)) + - ranges::size(get(parent_->views_))); + auto prev_size = ranges::distance(get(parent_->views_)); + it_.template emplace(ranges::begin(get(parent_->views_)) + prev_size); advance_bwd( - static_cast(ranges::size(get(parent_->views_))), + static_cast(prev_size), steps - current_offset); } } @@ -389,11 +391,11 @@ class concat_view : public view_interface> { requires xo::concat_random_access<__maybe_const...> { if (n > 0) { xo::visit_i(it_, [this, n](auto I, auto&& it) { - this->advance_fwd(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(it - ranges::begin(get(parent_->views_)), -n); + this->advance_bwd(static_cast(it - ranges::begin(get(parent_->views_))), -n); }); } return *this; @@ -477,11 +479,11 @@ 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] - (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 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; @@ -490,7 +492,7 @@ class concat_view : public view_interface> { return -(y - x); } else { return xo::visit_i(x.it_, - [&](auto I, auto&&) { return get(x.it_) - get(y.it_); }); + [&](auto I, auto&&) { return static_cast(get(x.it_) - get(y.it_)); }); } } @@ -507,7 +509,7 @@ 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] - (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); }