Skip to content

Commit

Permalink
address LWG feedback static_cast, iter_swap
Browse files Browse the repository at this point in the history
  • Loading branch information
@huixie90
huixie90 committed Sep 8, 2023
1 parent feb3899 commit dc57517
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 96 deletions.
85 changes: 41 additions & 44 deletions concat.lwg.feedback.20230906.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,48 +9,45 @@
TS: that is just stylistic
JW: it uses less exposition-only stuff
(swappable_with<iter_reference_t<iterator>, iter_reference_t<iterator>> && indirectly_swappable<iterator_t<maybe-const<Const, Views>>> && …)
[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<maybe-const<Const, Views>> 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<maybe-const<Const, Views>> 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<maybe-const<Const, Views>> 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<maybe-const<Const, Views>> 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
57 changes: 26 additions & 31 deletions concat.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ constexpr @_iterator_@<true> 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;
```
Expand All @@ -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<N - 1>,
ranges::end(get<N - 1>(@*views_*@))};
return @_iterator_@<@_is-const_@>(this, in_place_index<N - 1>,
ranges::end(get<N - 1>(@*views_*@)));
} else {
return default_sentinel;
}
Expand Down Expand Up @@ -982,11 +982,11 @@ if constexpr (N == sizeof...(Views) - 1) {
get<N>(@*it_*@) += static_cast<underlying_diff_type>(steps);
} else {
auto n_size = ranges::distance(get<N>(@*parent_*@->@*views_*@));
if (offset + steps < static_cast<difference_type>(n_size)) {
if (offset + steps < n_size) {
get<N>(@*it_*@) += static_cast<underlying_diff_type>(steps);
} else {
@*it_*@.template emplace<N + 1>(ranges::begin(get<N + 1>(@*parent_*@->@*views_*@)));
@*advance-fwd*@<N + 1>(0, offset + steps - static_cast<difference_type>(n_size));
@*advance-fwd*@<N + 1>(0, offset + steps - n_size);
}
}
```
Expand All @@ -1012,9 +1012,7 @@ if constexpr (N == 0) {
} else {
auto prev_size = ranges::distance(get<N - 1>(@*parent_*@->@*views_*@));
@*it_*@.template emplace<N - 1>(ranges::begin(get<N - 1>(@*parent_*@->@*views_*@)) + prev_size);
@*advance-bwd*@<N - 1>(
static_cast<difference_type>(prev_size),
steps - offset);
@*advance-bwd*@<N - 1>(prev_size, steps - offset);
}
}
```
Expand Down Expand Up @@ -1164,9 +1162,9 @@ constexpr @_iterator_@& operator+=(difference_type n)

```cpp
if(n > 0) {
@*advance-fwd*@<@*i*@>(static_cast<difference_type(get<@*i*@>(@*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<difference_type(get<@*i*@>(@*it_*@) - ranges::begin(get<@*i*@>(@*parent_*@->@*views_*@))), -n);
@*advance-bwd*@<@*i*@>(get<@*i*@>(@*it_*@) - ranges::begin(get<@*i*@>(@*parent_*@->@*views_*@)), -n);
}
return *this;
```
Expand Down Expand Up @@ -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*@<common_type_t<range_size_t<@*maybe-const*@<Const, Views>>...>>`,
equivalent to

```cpp
return static_cast<difference_type>(@*d~y~*@) + s + static_cast<difference_type>(@*d~x~*@);
return @*d~y~*@ + static_cast<difference_type>(s) + @*d~x~*@;
```

- [34.2]{.pnum} otherwise, if `@*i~x~*@ < @*i~y~*@`, equivalent to:
Expand All @@ -1351,7 +1350,7 @@ denote `y.@*it_*@.index()`
- [34.3]{.pnum} otherwise, equivalent to:

```cpp
return static_cast<difference_type>(get<@*i~x~*@>(x.@*it_*@) - get<@*i~y~*@>(y.@*it_*@));
return get<@*i~x~*@>(x.@*it_*@) - get<@*i~y~*@>(y.@*it_*@);
```

:::
Expand All @@ -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*@<common_type_t<range_size_t<@*maybe-const*@<Const, Views>>...>>`,
equivalent to

```cpp
return -(static_cast<difference_type>(@*d~x~*@) + static_cast<difference_type>(s));
return -(@*d~x~*@ + static_cast<difference_type>(s));
```

[37]{.pnum} *Remarks*: Let `V` be the last element of the pack `Views`, the expression in the requires-clause is equivalent to:
Expand Down Expand Up @@ -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, Views>> const` respectively.

[46]{.pnum} *Remarks*: The expression in the requires-clause is equivalent to

Expand Down
35 changes: 14 additions & 21 deletions impl/concat/concat.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,13 +318,13 @@ class concat_view : public view_interface<concat_view<Views...>> {
static_assert(
std::ranges::sized_range<decltype(get<N>(parent_->views_))>);
auto n_size = ranges::distance(get<N>(parent_->views_));
if (current_offset + steps < static_cast<difference_type>(n_size)) {
if (current_offset + steps < n_size) {
get<N>(it_) += static_cast<underlying_diff_type>(steps);
} else {
it_.template emplace<N + 1>(
ranges::begin(get<N + 1>(parent_->views_)));
advance_fwd<N + 1>(
0, current_offset + steps - static_cast<difference_type>(n_size));
0, current_offset + steps - n_size);
}
}
}
Expand All @@ -345,8 +345,7 @@ class concat_view : public view_interface<concat_view<Views...>> {
auto prev_size = ranges::distance(get<N - 1>(parent_->views_));
it_.template emplace<N - 1>(
ranges::begin(get<N - 1>(parent_->views_)) + prev_size);
advance_bwd<N - 1>(static_cast<difference_type>(prev_size),
steps - current_offset);
advance_bwd<N - 1>(prev_size, steps - current_offset);
}
}
}
Expand Down Expand Up @@ -421,15 +420,11 @@ class concat_view : public view_interface<concat_view<Views...>> {
requires xo::concat_is_random_access<Const, Views...> {
if (n > 0) {
xo::visit_i(it_, [this, n](auto I, auto&& it) {
this->advance_fwd<I>(static_cast<difference_type>(
it - ranges::begin(get<I>(parent_->views_))),
n);
this->advance_fwd<I>(it - ranges::begin(get<I>(parent_->views_)), n);
});
} else if (n < 0) {
xo::visit_i(it_, [this, n](auto I, auto&& it) {
this->advance_bwd<I>(static_cast<difference_type>(
it - ranges::begin(get<I>(parent_->views_))),
-n);
this->advance_bwd<I>(it - ranges::begin(get<I>(parent_->views_)), -n);
});
}
return *this;
Expand Down Expand Up @@ -536,13 +531,11 @@ class concat_view : public view_interface<concat_view<Views...>> {

auto y_to_end = xo::visit_i(y.it_, [&](auto I, auto&& it) {
return all_sizes[I] -
(static_cast<difference_type>(
it - ranges::begin(get<I>(y.get_parent_views()))));
(it - ranges::begin(get<I>(y.get_parent_views())));
});

auto begin_to_x = xo::visit_i(x.it_, [&](auto I, auto&& it) {
return static_cast<difference_type>(
it - ranges::begin(get<I>(x.get_parent_views())));
return it - ranges::begin(get<I>(x.get_parent_views()));
});

return y_to_end + in_between + begin_to_x;
Expand All @@ -551,7 +544,7 @@ class concat_view : public view_interface<concat_view<Views...>> {
return -(y - x);
} else {
return xo::visit_i(x.it_, [&](auto I, auto&&) {
return static_cast<difference_type>(get<I>(x.it_) - get<I>(y.it_));
return get<I>(x.it_) - get<I>(y.it_);
});
}
}
Expand Down Expand Up @@ -627,7 +620,7 @@ class concat_view : public view_interface<concat_view<Views...>> {

constexpr iterator<false> begin() requires(!(__simple_view<Views> && ...)) //
{
iterator<false> it{this, in_place_index<0u>, ranges::begin(get<0>(views_))};
iterator<false> it(this, in_place_index<0u>, ranges::begin(get<0>(views_)));
it.template satisfy<0>();
return it;
}
Expand All @@ -636,7 +629,7 @@ class concat_view : public view_interface<concat_view<Views...>> {
requires((range<const Views> && ...) &&
xo::concatable<const Views...>) //
{
iterator<true> it{this, in_place_index<0u>, ranges::begin(get<0>(views_))};
iterator<true> it(this, in_place_index<0u>, ranges::begin(get<0>(views_)));
it.template satisfy<0>();
return it;
}
Expand All @@ -645,8 +638,8 @@ class concat_view : public view_interface<concat_view<Views...>> {
using LastView = xo::back<Views...>;
if constexpr (common_range<LastView>) {
constexpr auto N = sizeof...(Views);
return iterator<false>{this, in_place_index<N - 1>,
ranges::end(get<N - 1>(views_))};
return iterator<false>(this, in_place_index<N - 1>,
ranges::end(get<N - 1>(views_)));
} else {
return default_sentinel;
}
Expand All @@ -656,8 +649,8 @@ class concat_view : public view_interface<concat_view<Views...>> {
using LastView = xo::back<const Views...>;
if constexpr (common_range<LastView>) {
constexpr auto N = sizeof...(Views);
return iterator<true>{this, in_place_index<N - 1>,
ranges::end(get<N - 1>(views_))};
return iterator<true>(this, in_place_index<N - 1>,
ranges::end(get<N - 1>(views_)));
} else {
return default_sentinel;
}
Expand Down

0 comments on commit dc57517

Please sign in to comment.