From dc57517439a2f93e2eadd2d5550bc3916dbd67e3 Mon Sep 17 00:00:00 2001 From: Hui Date: Fri, 8 Sep 2023 20:28:25 +0100 Subject: [PATCH] address LWG feedback static_cast, iter_swap --- concat.lwg.feedback.20230906.md | 85 ++++++++++++++++----------------- concat.md | 57 ++++++++++------------ impl/concat/concat.hpp | 35 ++++++-------- 3 files changed, 81 insertions(+), 96 deletions(-) diff --git a/concat.lwg.feedback.20230906.md b/concat.lwg.feedback.20230906.md index 2090233..755b4b2 100644 --- a/concat.lwg.feedback.20230906.md +++ b/concat.lwg.feedback.20230906.md @@ -9,48 +9,45 @@ TS: that is just stylistic JW: it uses less exposition-only stuff (swappable_with, iter_reference_t> && indirectly_swappable>> && …) -[TODO] TK: there is a rewording in the chat avoiding N: -[TODO] “The exception specfication is equivalent to the logical AND of noexcept(ranges::swap(...)) and the following expression for every integer …” -[TODO] JW: I think I prefer that because we start with what we are saying -[TODO] TS: we need to reword that because std::get is actually throwing -[TODO] TS: we don’t care about std::get throwing here, though -[TODO] JW: I don’t have a quick solution to that! That will need some work off-line -[TODO] TS: we could use get_if but that is ugly: using a * and taking the address with addressof. -[TODO] TS: we could use declval and write it as a full expression -[TODO] TK: another suggestion in the chat -[TODO] JW: that would be a pack of types, i.e., we’d need to declval it - OK, now corrected: -[TODO] “noexcept(ranges::iter_swap(IT, IT)) for IT being lvalue of type iterator_t> const, for each type in the pack.” -[TODO] BST: should we use “conjunction” instead of “logical AND” -[TODO] DK: I prefer logical and because I know what it means; conjunction I keep needing to look up -[TODO] TK: wording for p4 in the chat: -[TODO] “Exception specification is equivalent to: -[TODO] noexcept(ranges::swap(*x, *y)) && ... && noexcept(ranges::iter_swap(its, its)) -[TODO] where its is a pack of lvalues of type iterator_t> const respectively.” -[TODO] HX: in the previous wording all ranges needed to be sized, now the last range doesn’t need to be sized -[TODO] JW: the last range doesn’t have to be sized because we don’t skip to the end of it -[TODO] JW: is there an efficient way to get the last element of a pack -[TODO] SLY: there is a simple implementation in the paper, see https://godbolt.org/z/TG8MhbWqq +[Done] TK: there is a rewording in the chat avoiding N: + “The exception specfication is equivalent to the logical AND of noexcept(ranges::swap(...)) and the following expression for every integer …” + JW: I think I prefer that because we start with what we are saying + TS: we need to reword that because std::get is actually throwing + TS: we don’t care about std::get throwing here, though + JW: I don’t have a quick solution to that! That will need some work off-line + TS: we could use get_if but that is ugly: using a * and taking the address with addressof. + TS: we could use declval and write it as a full expression + TK: another suggestion in the chat + JW: that would be a pack of types, i.e., we’d need to declval it - OK, now corrected: + “noexcept(ranges::iter_swap(IT, IT)) for IT being lvalue of type iterator_t> const, for each type in the pack.” + BST: should we use “conjunction” instead of “logical AND” + DK: I prefer logical and because I know what it means; conjunction I keep needing to look up + TK: wording for p4 in the chat: + “Exception specification is equivalent to: + noexcept(ranges::swap(*x, *y)) && ... && noexcept(ranges::iter_swap(its, its)) + where its is a pack of lvalues of type iterator_t> const respectively.” [TODO] TK: on concat-is-bidirectional: we check it twice -[TODO] TK: we have similar things in the cartesian_view: we could extract the logic for cartesian-is-common -[TODO] JW: that makes sense -[TODO] TS: (2.2.4) p5, p6 “offset + steps < …” doesn’t need a static_cast: you can do signed integer comparison with different sizes, i.e., this one doesn’t needs a cast -[TODO] TS: I think the advance-fwd thing also doesn’t need a static_cast: either it produces the right type or we are in integer land -[TODO] HX: I think the issue was that the conversion was explicit -[TODO] TS: only when you are possibly truncating but widening does not -[TODO] JW: but without the cast we may end up with the wrong type -[TODO] TS: they are on the same type -[TODO] TS: the two casts which are required is on the addition to the iterator -[TODO] JW: so remove for all that are not += and the equivalence a bit further on (-=) -[TODO] TK: begin()/end() use {} to initialize it: they should use () -[TODO] JW: we only use {} if we really want to prevent narrowing conversions -[TODO] TS: on p19: the static_cast isn’t necessary: the conversion is implicit -[TODO] JW: it also lacks the > -[TODO] TS: on p34.1: I don’t think it needs a static_cast but it needs to say what type s is. -[TODO] TS: there is one s total not one s per integer -[TODO] TS: it is probably easier to just static_cast s -[TODO] TK: we may overflow: the difference type may not be able to hold that -[TODO] TS: if the difference type can’t represent the distance between two iterators that is already broken even without using - -[TODO] TS: on p34.3: the static_cast is unnecessary -[TODO] JW: same on p36 -[TODO] TS: that has the same issue with having just one s and it doesn’t actually say what the type is -[TODO] TS: I don’t see why we say “denotes the difference” instead of using the expression + TK: we have similar things in the cartesian_view: we could extract the logic for cartesian-is-common + JW: that makes sense + +[Done] TS: (2.2.4) p5, p6 “offset + steps < …” doesn’t need a static_cast: you can do signed integer comparison with different sizes, i.e., this one doesn’t needs a cast + TS: I think the advance-fwd thing also doesn’t need a static_cast: either it produces the right type or we are in integer land + HX: I think the issue was that the conversion was explicit + TS: only when you are possibly truncating but widening does not + JW: but without the cast we may end up with the wrong type + TS: they are on the same type + TS: the two casts which are required is on the addition to the iterator + JW: so remove for all that are not += and the equivalence a bit further on (-=) +[Done] TK: begin()/end() use {} to initialize it: they should use () + JW: we only use {} if we really want to prevent narrowing conversions +[Done] TS: on p19: the static_cast isn’t necessary: the conversion is implicit + JW: it also lacks the > +[Done] TS: on p34.1: I don’t think it needs a static_cast but it needs to say what type s is. + TS: there is one s total not one s per integer + TS: it is probably easier to just static_cast s + TK: we may overflow: the difference type may not be able to hold that + TS: if the difference type can’t represent the distance between two iterators that is already broken even without using - +[Done] TS: on p34.3: the static_cast is unnecessary +[Done] JW: same on p36 + TS: that has the same issue with having just one s and it doesn’t actually say what the type is +[Done] TS: I don’t see why we say “denotes the difference” instead of using the expression diff --git a/concat.md b/concat.md index 6cf9c7c..2e83485 100644 --- a/concat.md +++ b/concat.md @@ -718,7 +718,7 @@ constexpr @_iterator_@ begin() const and `false` otherwise. Equivalent to: ```cpp -@_iterator_@<@_is-const_@> it{this, in_place_index<0>, ranges::begin(get<0>(@*views_*@))}; +@_iterator_@<@_is-const_@> it(this, in_place_index<0>, ranges::begin(get<0>(@*views_*@))); it.template @_satisfy_@<0>(); return it; ``` @@ -740,8 +740,8 @@ and `false` otherwise, and let `@_last-view_@` be the last element of the pack ```cpp if constexpr (common_range<@_last-view_@>) { constexpr auto N = sizeof...(Views); - return @_iterator_@<@_is-const_@>{this, in_place_index, - ranges::end(get(@*views_*@))}; + return @_iterator_@<@_is-const_@>(this, in_place_index, + ranges::end(get(@*views_*@))); } else { return default_sentinel; } @@ -982,11 +982,11 @@ if constexpr (N == sizeof...(Views) - 1) { get(@*it_*@) += static_cast(steps); } else { auto n_size = ranges::distance(get(@*parent_*@->@*views_*@)); - if (offset + steps < static_cast(n_size)) { + if (offset + steps < n_size) { get(@*it_*@) += static_cast(steps); } else { @*it_*@.template emplace(ranges::begin(get(@*parent_*@->@*views_*@))); - @*advance-fwd*@(0, offset + steps - static_cast(n_size)); + @*advance-fwd*@(0, offset + steps - n_size); } } ``` @@ -1012,9 +1012,7 @@ if constexpr (N == 0) { } 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 - offset); + @*advance-bwd*@(prev_size, steps - offset); } } ``` @@ -1164,9 +1162,9 @@ constexpr @_iterator_@& operator+=(difference_type n) ```cpp if(n > 0) { - @*advance-fwd*@<@*i*@>(static_cast(@*it_*@) - ranges::begin(get<@*i*@>(@*parent_*@->@*views_*@))), n); + @*advance-fwd*@<@*i*@>(get<@*i*@>(@*it_*@) - ranges::begin(get<@*i*@>(@*parent_*@->@*views_*@)), n); } else if (n < 0) { - @*advance-bwd*@<@*i*@>(static_cast(@*it_*@) - ranges::begin(get<@*i*@>(@*parent_*@->@*views_*@))), -n); + @*advance-bwd*@<@*i*@>(get<@*i*@>(@*it_*@) - ranges::begin(get<@*i*@>(@*parent_*@->@*views_*@)), -n); } return *this; ``` @@ -1329,17 +1327,18 @@ friend constexpr difference_type operator-(const @_iterator_@& x, const @_iterat [34]{.pnum} *Effects*: Let `@*i~x~*@` denote `x.@*it_*@.index()` and `@*i~y~*@` denote `y.@*it_*@.index()` -- [34.1]{.pnum} if `@*i~x~*@ > @*i~y~*@`, let `@*d~y~*@` denote the distance - from `get<@*i~y~*@>(y.@*it_*@)` to the end of - `get<@*i~y~*@>(y.@*parent_*@.@*views_*@)`, `@*d~x~*@` denote the distance from - the begin of `get<@*i~x~*@>(x.@*parent_*@.@*views_*@)` to - `get<@*i~x~*@>(x.@*it_*@)`. For every integer `@*i~y~*@ < @*i*@ < @*i~x~*@`, +- [34.1]{.pnum} if `@*i~x~*@ > @*i~y~*@`, let `@*d~y~*@` be + `ranges::distance(get<@*i~y~*@>(y.@*it_*@), ranges::end(get<@*i~y~*@>(y.@*parent_*@->@*views_*@)))`, + `@*d~x~*@` be + `ranges::distance(ranges::begin(get<@*i~x~*@>(x.@*parent_*@->@*views_*@)), get<@*i~x~*@>(x.@*it_*@))`. + For every integer `@*i~y~*@ < @*i*@ < @*i~x~*@`, let `s` denote the sum of the sizes of all the ranges - `get<@*i*@>(x.@*parent_*@.@*views_*@)` if there is any, and `0` otherwise, + `get<@*i*@>(x.@*parent_*@->@*views_*@)` if there is any, and `0` otherwise, of type + `@*make-unsigned-like-t*@>...>>`, equivalent to ```cpp - return static_cast(@*d~y~*@) + s + static_cast(@*d~x~*@); + return @*d~y~*@ + static_cast(s) + @*d~x~*@; ``` - [34.2]{.pnum} otherwise, if `@*i~x~*@ < @*i~y~*@`, equivalent to: @@ -1351,7 +1350,7 @@ denote `y.@*it_*@.index()` - [34.3]{.pnum} otherwise, equivalent to: ```cpp - return static_cast(get<@*i~x~*@>(x.@*it_*@) - get<@*i~y~*@>(y.@*it_*@)); + return get<@*i~x~*@>(x.@*it_*@) - get<@*i~y~*@>(y.@*it_*@); ``` ::: @@ -1366,14 +1365,14 @@ friend constexpr difference_type operator-(const @_iterator_@& x, default_sentin [35]{.pnum} *Preconditions*: `x.@*it_*@.valueless_by_exception()` is `false`. [36]{.pnum} *Effects*: Let `@*i~x~*@` denote `x.@*it_*@.index()`, `@*d~x~*@` -denote the distance from `get<@*i~x~*@>(x.@*it_*@)` to the end of -`get<@*i~x~*@>(x.@*parent_*@.@*views_*@)`. For every integer -`@*i~x~*@ < @*i*@ < sizeof...(Views)`, let `s` denote the sum of the sizes of -all the ranges `get<@*i*@>(x.@*parent_*@.@*views_*@)` if there is any, and `0` -otherwise, equivalent to +be `ranges::distance(get<@*i~x~*@>(x.@*it_*@), ranges::end(get<@*i~x~*@>(x.@*parent_*@->@*views_*@)))`. +For every integer `@*i~x~*@ < @*i*@ < sizeof...(Views)`, let `s` denote the sum of the sizes of +all the ranges `get<@*i*@>(x.@*parent_*@->@*views_*@)` if there is any, and `0` +otherwise, of type `@*make-unsigned-like-t*@>...>>`, +equivalent to ```cpp -return -(static_cast(@*d~x~*@) + static_cast(s)); +return -(@*d~x~*@ + static_cast(s)); ``` [37]{.pnum} *Remarks*: Let `V` be the last element of the pack `Views`, the expression in the requires-clause is equivalent to: @@ -1460,17 +1459,13 @@ std::visit( x.@*it_*@, y.@*it_*@); ``` -[45]{.pnum} *Remarks*: Let `N` be the logical `AND` of the following expressions: +[45]{.pnum} *Remarks*: The exception specification is equivalent to ```cpp -noexcept(ranges::iter_swap(std::get<@*i*@>(x.@*it_*@), std::get<@*i*@>(y.@*it_*@))) +(noexcept(ranges::swap(*x, *y)) && ... && noexcept(ranges::iter_swap(its, its))) ``` -for every integer 0 <= `@*i*@` < `sizeof...(Views)`, the exception specification is equivalent to - -```cpp -noexcept(ranges::swap(*x, *y)) && N -``` +where `its` is a pack of lvalues of type `iterator_t<@*maybe-const*@> const` respectively. [46]{.pnum} *Remarks*: The expression in the requires-clause is equivalent to diff --git a/impl/concat/concat.hpp b/impl/concat/concat.hpp index 40a7048..845cd10 100644 --- a/impl/concat/concat.hpp +++ b/impl/concat/concat.hpp @@ -318,13 +318,13 @@ class concat_view : public view_interface> { static_assert( std::ranges::sized_range(parent_->views_))>); auto n_size = ranges::distance(get(parent_->views_)); - if (current_offset + steps < static_cast(n_size)) { + if (current_offset + steps < n_size) { get(it_) += static_cast(steps); } else { it_.template emplace( ranges::begin(get(parent_->views_))); advance_fwd( - 0, current_offset + steps - static_cast(n_size)); + 0, current_offset + steps - n_size); } } } @@ -345,8 +345,7 @@ class concat_view : public view_interface> { 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); + advance_bwd(prev_size, steps - current_offset); } } } @@ -421,15 +420,11 @@ class concat_view : public view_interface> { 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(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(it - ranges::begin(get(parent_->views_)), -n); }); } return *this; @@ -536,13 +531,11 @@ class concat_view : public view_interface> { 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())))); + (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 it - ranges::begin(get(x.get_parent_views())); }); return y_to_end + in_between + begin_to_x; @@ -551,7 +544,7 @@ class concat_view : public view_interface> { return -(y - x); } else { return xo::visit_i(x.it_, [&](auto I, auto&&) { - return static_cast(get(x.it_) - get(y.it_)); + return get(x.it_) - get(y.it_); }); } } @@ -627,7 +620,7 @@ class concat_view : public view_interface> { constexpr iterator begin() requires(!(__simple_view && ...)) // { - iterator it{this, in_place_index<0u>, ranges::begin(get<0>(views_))}; + iterator it(this, in_place_index<0u>, ranges::begin(get<0>(views_))); it.template satisfy<0>(); return it; } @@ -636,7 +629,7 @@ class concat_view : public view_interface> { requires((range && ...) && xo::concatable) // { - iterator it{this, in_place_index<0u>, ranges::begin(get<0>(views_))}; + iterator it(this, in_place_index<0u>, ranges::begin(get<0>(views_))); it.template satisfy<0>(); return it; } @@ -645,8 +638,8 @@ class concat_view : public view_interface> { using LastView = xo::back; if constexpr (common_range) { constexpr auto N = sizeof...(Views); - return iterator{this, in_place_index, - ranges::end(get(views_))}; + return iterator(this, in_place_index, + ranges::end(get(views_))); } else { return default_sentinel; } @@ -656,8 +649,8 @@ class concat_view : public view_interface> { using LastView = xo::back; if constexpr (common_range) { constexpr auto N = sizeof...(Views); - return iterator{this, in_place_index, - ranges::end(get(views_))}; + return iterator(this, in_place_index, + ranges::end(get(views_))); } else { return default_sentinel; }