Skip to content

Commit

Permalink
update wording for dropping !common && random_access && sized
Browse files Browse the repository at this point in the history
  • Loading branch information
huixie90 committed Sep 23, 2023
1 parent e6799d0 commit bee7554
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 116 deletions.
6 changes: 3 additions & 3 deletions concat.lwg.feedback.20230913.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
```
[TODO] TK: common-arg is very generic (it is exposition-only); can we give a different name?
[REMOVED] TK: common-arg is very generic (it is exposition-only); can we give a different name?
JW: people should think of a better name - we don’t do that now
[DONE] JW: maybe_const should be hyphenated
[TODO] TK: [on advance-bwd] we always compute the end but we don’t need to do that for common; in Cartesian product there is something like that; the question is whether we want to do it here, too?
[DONE] TK: [on advance-bwd] we always compute the end but we don’t need to do that for common; in Cartesian product there is something like that; the question is whether we want to do it here, too?
JW: anybody want to have that replaced by the tool from Cartesian?
JW: implementations can do that anyway
TK: equivalent => the + is observable
Expand All @@ -21,7 +21,7 @@
TK: why would we impose a requirement which isn’t needed
TS: you don’t need random access but you can work around that
JW: is this an SG9 discussion? That seems to be a design question
[TODO] TK: same discussion: the ranges::end(...) isn’t required to be sized, i.e., we need to use a common range again
[Done] TK: same discussion: the ranges::end(...) isn’t required to be sized, i.e., we need to use a common range again
TK: I guess, that is the reason why we shouldn’t bother
JG: aside from that point, let’s check if we are happy with the changes
TK: it should be back to the prose if we need the end
Expand Down
91 changes: 20 additions & 71 deletions concat.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ toc: true

## R6

- Added a section `!common_range && random_access_range && sized_range`
- remove `bidirectional_range` support for `!common_range && random_access_range && sized_range`
- remove `random_access_range` support for `!common_range && random_access_range && sized_range`

## R5

Expand Down Expand Up @@ -408,6 +409,8 @@ only concepts and functions can be reused for both of the views, and their defin
of `end` function would be very similar, except that `cartesian_product_view` checks
the first underlying range and `concat_view` checks the last underlying range.
As per SG9 direction, this section is dropped.
### `!common_range && (random_accessed_range && sized_range)`
In R0 version, `concat_view` is a `common_range` if the last range satisfies
Expand Down Expand Up @@ -477,6 +480,7 @@ access support in the `concat_view`, they can do
auto cv = views::concat(r | views::common, other_views...)
```

As per LEWG direction, the change proposed in this section is adopted
## Bidirectional Range

`concat_view` can model `bidirectional_range` if the underlying ranges satisfy
Expand All @@ -490,7 +494,7 @@ the following conditions:
`--ranges::end(n-1th range)`, assuming n-1th range is not empty, or
- `random_access_range && sized_range`, so the position can be reached by
`ranges::begin(n-1th range) + (ranges::size(n-1th range) - 1)` in constant
time, assuming n-1th range is not empty.
time, assuming n-1th range is not empty. However, as per LEWG's direction,`!common_range && random_access_range && sized_range` is removed

Note that the last underlying range does not have to satisfy this constraint
because n-1 can never be the last underlying range. If neither of the two
Expand All @@ -502,15 +506,15 @@ In the `concat` implementation in [@rangev3], `operator--` is only constrained
on all underlying ranges being `bidirectional_range` on the declaration, but its
implementation is using `ranges::next(ranges::begin(r), ranges::end(r))` which
implicitly requires random access to make the operation constant time. So it
went with the second constraint. In this paper, both are supported.
went with the second constraint.

## Random Access Range

`concat_view` can model `random_access_range` if the underlying ranges satisfy
the following conditions:

- Every underlying range models `random_access_range`
- All except the last range model `sized_range`
- All except the last range model `common_range`

## Sized Range

Expand Down Expand Up @@ -667,54 +671,6 @@ namespace std::ranges {
}
```

Add the following to [range.adaptor.helpers]{.sref}

```diff
namespace std::ranges {
// [...]
+ template<class R>
+ concept @*common-arg*@ = // exposition only
+ common_range<R> || (sized_range<R> && random_access_range<R>);
// [...]
}
```

Modify the following in [range.cartesian.view]{.sref}

```diff
namespace std::ranges {
// [...]
- template<class R>
- concept @*cartesian-product-common-arg*@ = // exposition only
- common_range<R> || (sized_range<R> && random_access_range<R>);

template<bool Const, class First, class... Vs>
concept @*cartesian-product-is-bidirectional*@ = // exposition only
(bidirectional_range<@*maybe-const*@<Const, First>> && ... &&
(bidirectional_range<@*maybe-const*@<Const, Vs>>
- && @*cartesian-product-common-arg*@<@*maybe-const*@<Const, Vs>>));
+ && @*common-arg*@<@*maybe-const*@<Const, Vs>>));

template<class First, class... Vs>
concept @*cartesian-product-is-common*@ = // exposition only
- @*cartesian-product-common-arg*@<First>;
+ @*common-arg*@<First>;

// [...]

- template<@*cartesian-product-common-arg*@ R>
+ template<@*common-arg*@ R>
constexpr auto @*cartesian-common-arg-end*@(R& r) { // exposition only
if constexpr (common_range<R>) {
return ranges::end(r);
} else {
return ranges::begin(r) + ranges::distance(r);
}
}
// [...]
}
```

## `concat`

Add the following subclause to [range.adaptors]{.sref}.
Expand Down Expand Up @@ -871,7 +827,7 @@ concept @_concat-is-random-access_@ = @*see below*@; // exposition only
template <bool Const, class... Rs>
concept @_concat-is-random-access_@ = // exposition only
@*all-random-access*@<Const, Rs...> &&
(sized_range<@*maybe-const*@<Const, Fs>> && ...);
(common_range<@*maybe-const*@<Const, Fs>> && ...);
```
:::
Expand All @@ -888,7 +844,7 @@ concept @_concat-is-bidirectional_@ = @*see below*@; // exposition only
```cpp
template <bool Const, class... Rs>
concept @_concat-is-bidirectional_@ = // exposition only
(@*all-bidirectional*@<Const, Rs...> && ... && @*common-arg*@<@*maybe-const*@<Const, Fs>>);
(@*all-bidirectional*@<Const, Rs...> && ... && common_range<@*maybe-const*@<Const, Fs>>);
```
:::
Expand Down Expand Up @@ -1147,14 +1103,7 @@ if constexpr (N == 0) {
--get<0>(@*it_*@);
} else {
if (get<N>(@*it_*@) == ranges::begin(get<N>(@*parent_*@->@*views_*@))) {
using prev_view = @_maybe-const_@<Const, tuple_element_t<N - 1, tuple<Views...>>>;
if constexpr (common_range<prev_view>) {
@*it_*@.template emplace<N - 1>(ranges::end(get<N - 1>(@*parent_*@->@*views_*@)));
} else {
@*it_*@.template emplace<N - 1>(
ranges::next(ranges::begin(get<N - 1>(@*parent_*@->@*views_*@)),
ranges::size(get<N - 1>(@*parent_*@->@*views_*@))));
}
@*it_*@.template emplace<N - 1>(ranges::end(get<N - 1>(@*parent_*@->@*views_*@)));
@_prev_@<N - 1>();
} else {
--get<N>(@*it_*@);
Expand Down Expand Up @@ -1208,7 +1157,7 @@ if constexpr (N == 0) {
get<N>(@*it_*@) -= static_cast<underlying_diff_type>(steps);
} 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);
@*it_*@.template emplace<N - 1>(ranges::end(get<N - 1>(@*parent_*@->@*views_*@)));
@*advance-bwd*@<N - 1>(prev_size, steps - offset);
}
}
Expand Down Expand Up @@ -1525,17 +1474,17 @@ friend constexpr difference_type operator-(const @_iterator_@& x, const @_iterat
denote `y.@*it_*@.index()`

- [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_*@)))`,
`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_*@))`.
Let `s` denote the sum of the sizes of all the ranges
`get<@*i*@>(x.@*parent_*@->@*views_*@)` for every integer
`@*i~y~*@ < @*i*@ < @*i~x~*@` if there is any, and `0` otherwise, of type
`@*make-unsigned-like-t*@<common_type_t<range_size_t<@*maybe-const*@<Const, Views>>...>>`,
`difference_type`,
equivalent to

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

- [34.2]{.pnum} otherwise, if `@*i~x~*@ < @*i~y~*@`, equivalent to:
Expand Down Expand Up @@ -1564,19 +1513,19 @@ friend constexpr difference_type operator-(const @_iterator_@& x, default_sentin
[36]{.pnum} *Effects*: Let `@*i~x~*@` denote `x.@*it_*@.index()`, `@*d~x~*@`
be `ranges::distance(get<@*i~x~*@>(x.@*it_*@), ranges::end(get<@*i~x~*@>(x.@*parent_*@->@*views_*@)))`.
Let `s` denote the sum of the sizes of all the ranges
`get<@*i*@>(x.@*parent_*@->@*views_*@)` For every integer
`get<@*i*@>(x.@*parent_*@->@*views_*@)` for every integer
`@*i~x~*@ < @*i*@ < sizeof...(Views)` if there is any, and `0`
otherwise, of type `@*make-unsigned-like-t*@<common_type_t<range_size_t<@*maybe-const*@<Const, Views>>...>>`,
otherwise, of type `difference_type`,
equivalent to

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

[37]{.pnum} *Remarks*: Let `V` be the last element of the pack `Views`, the expression in the requires-clause is equivalent to:

```cpp
(@_concat-is-random-access_@<Const, Views...> && sized_range<@*maybe-const*@<Const, V>>)
(@_concat-is-random-access_@<Const, Views...> && common_range<@*maybe-const*@<Const, V>>)
```

:::
Expand All @@ -1597,7 +1546,7 @@ return -(x - default_sentinel);
[39]{.pnum} *Remarks*: Let `V` be the last element of the pack `Views`, the expression in the requires-clause is equivalent to:

```cpp
(@_concat-is-random-access_@<Const, Views...> && sized_range<@*maybe-const*@<Const, V>>)
(@_concat-is-random-access_@<Const, Views...> && common_range<@*maybe-const*@<Const, V>>)
```

:::
Expand Down
44 changes: 13 additions & 31 deletions impl/concat/concat.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ concept all_bidirectional = (bidirectional_range<__maybe_const<Const, Views>> &&
template <bool Const, class... Views>
concept all_forward = (forward_range<__maybe_const<Const, Views>> && ...);

template<class R>
concept common_arg = // exposition only
common_range<R> || (sized_range<R> && random_access_range<R>);

inline namespace not_to_spec {

template <class... Rs>
Expand Down Expand Up @@ -88,10 +84,10 @@ constexpr bool all_but_last =
template <bool Const, class... Views>
concept concat_is_random_access =
(all_random_access<Const, Views> && ...) &&
(all_but_last<sized_range<__maybe_const<Const, Views>>...>);
(all_but_last<common_range<__maybe_const<Const, Views>>...>);

template <bool Const, class... Views>
concept concat_bidirectional = all_but_last<common_arg<__maybe_const<Const, Views>>...> &&
concept concat_bidirectional = all_but_last<common_range<__maybe_const<Const, Views>>...> &&
all_bidirectional<Const, Views...>;

static_assert(true); // clang-format badness
Expand Down Expand Up @@ -287,18 +283,8 @@ class concat_view : public view_interface<concat_view<Views...>> {
--get<0>(it_);
} else {
if (get<N>(it_) == ranges::begin(get<N>(parent_->views_))) {
using prev_view =
__maybe_const<Const, tuple_element_t<N - 1, tuple<Views...>>>;
if constexpr (common_range<prev_view>) {
it_.template emplace<N - 1>(
ranges::end(get<N - 1>(parent_->views_)));
} else {
static_assert(random_access_range<prev_view> &&
sized_range<prev_view>);
it_.template emplace<N - 1>(
ranges::next(ranges::begin(get<N - 1>(parent_->views_)),
ranges::size(get<N - 1>(parent_->views_))));
}
prev<N - 1>();
} else {
--get<N>(it_);
Expand All @@ -315,7 +301,7 @@ class concat_view : public view_interface<concat_view<Views...>> {
get<N>(it_) += static_cast<underlying_diff_type>(steps);
} else {
static_assert(
std::ranges::sized_range<decltype(get<N>(parent_->views_))>);
std::ranges::common_range<decltype(get<N>(parent_->views_))>);
auto n_size = ranges::distance(get<N>(parent_->views_));
if (current_offset + steps < n_size) {
get<N>(it_) += static_cast<underlying_diff_type>(steps);
Expand All @@ -340,10 +326,9 @@ class concat_view : public view_interface<concat_view<Views...>> {
get<N>(it_) -= static_cast<underlying_diff_type>(steps);
} else {
static_assert(
std::ranges::sized_range<decltype(get<N - 1>(parent_->views_))>);
std::ranges::common_range<decltype(get<N - 1>(parent_->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);
it_.template emplace<N - 1>(ranges::end(get<N - 1>(parent_->views_)));
advance_bwd<N - 1>(prev_size, steps - current_offset);
}
}
Expand Down Expand Up @@ -513,10 +498,10 @@ class concat_view : public view_interface<concat_view<Views...>> {
const auto all_sizes = std::apply(
[&](const auto&... views) {
const auto getSize = [](const auto& view) {
if constexpr (ranges::sized_range<decay_t<decltype(view)>>) {
return ranges::size(view);
if constexpr (ranges::common_range<decay_t<decltype(view)>>) {
return ranges::distance(view);
} else {
return 0u; // only the last range can be non-sized, and its
return 0; // only the last range can be non common, and its
// value is not used
}
};
Expand All @@ -529,8 +514,7 @@ class concat_view : public view_interface<concat_view<Views...>> {
difference_type(0));

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

auto begin_to_x = xo::visit_i(x.it_, [&](auto I, auto&& it) {
Expand All @@ -551,29 +535,27 @@ class concat_view : public view_interface<concat_view<Views...>> {
friend constexpr difference_type
operator-(const iterator& it, default_sentinel_t) requires(
xo::concat_is_random_access<Const, Views...>&&
sized_range<xo::back<__maybe_const<Const, Views>...>>) {
common_range<xo::back<__maybe_const<Const, Views>...>>) {
const auto idx = it.it_.index();
const auto all_sizes = std::apply(
[&](const auto&... views) {
return std::array{
static_cast<difference_type>(ranges::size(views))...};
static_cast<difference_type>(ranges::distance(views))...};
},
it.get_parent_views());
auto to_the_end = 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<difference_type>(
i - ranges::begin(get<I>(it.get_parent_views())));
return ranges::distance(i, ranges::end(get<I>(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<Const, Views...>&&
sized_range<xo::back<__maybe_const<Const, Views>...>>) {
common_range<xo::back<__maybe_const<Const, Views>...>>) {
return -(it - default_sentinel);
}

Expand Down
21 changes: 11 additions & 10 deletions impl/concat/test/basics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ TEST_POINT("bidirectional_concept") {
STATIC_REQUIRE(bidirectional_range<iota_view<int, int>>);
STATIC_REQUIRE(common_range<iota_view<int, int>>);

STATIC_CHECK(bidirectional_range<concat_view_of<iota_view<int, size_t>, IntV&>>); // because
STATIC_CHECK(!bidirectional_range<concat_view_of<iota_view<int, size_t>, IntV&>>); // because
STATIC_REQUIRE(bidirectional_range<iota_view<int, size_t>>);
STATIC_REQUIRE(!common_range<iota_view<int, size_t>>);
STATIC_REQUIRE(random_access_range<iota_view<int, size_t>>);
Expand Down Expand Up @@ -326,7 +326,7 @@ TEST_POINT("bidirectional_common") {
}


TEST_POINT("bidirectional_noncommon_random_access_sized") {
TEST_POINT("non-bidi_noncommon_random_access_sized") {
std::vector<int> v1{1};
std::ranges::empty_view<int> e2{};
std::ranges::iota_view<int, size_t> i3{0, 0};
Expand All @@ -345,14 +345,15 @@ TEST_POINT("bidirectional_noncommon_random_access_sized") {
++it;
REQUIRE(it == std::ranges::end(cv));

--it;
REQUIRE(*it == 3);
static_assert(!std::ranges::bidirectional_range<decltype(cv)>);
// --it;
// REQUIRE(*it == 3);

--it;
REQUIRE(*it == 2);
// --it;
// REQUIRE(*it == 2);

--it;
REQUIRE(*it == 1);
// --it;
// REQUIRE(*it == 1);
}


Expand All @@ -370,8 +371,8 @@ TEST_POINT("random_but_not_bidi_impossible") {

using CV = decltype(std::views::concat(NonCommonRandomSized{}, NonCommonRandomSized{}));
static_assert(!common_range<CV>);
static_assert(bidirectional_range<CV>);
static_assert(random_access_range<CV>);
static_assert(!bidirectional_range<CV>);
static_assert(!random_access_range<CV>);
static_assert(sized_range<CV>);
}

Expand Down
Loading

0 comments on commit bee7554

Please sign in to comment.