Skip to content

Commit

Permalink
address LWG feedback 20230913
Browse files Browse the repository at this point in the history
  • Loading branch information
huixie90 committed Sep 14, 2023
1 parent e46ff42 commit a42d0b9
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 13 deletions.
44 changes: 44 additions & 0 deletions concat.lwg.feedback.20230913.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
- [TODO] 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?
JW: anybody want to have that replaced by the tool from Cartesian?
JW: implementations can do that anyway
TK: equivalent => the + is observable
TS: the random access and not common is the odd one: we could require the range to just give it to you
TS: the treatment is inconsistent for this type of ranges
JW: so we would have another constexpr if ?
TK: yes and use the helper from Cartesian
HX: what do you about bidir ranges: also do the common thing?
TS: yes
HX: we could but I want to point out that such kind of ranges exist like iota(0, 1, l) is such a range
TK: in the prev() we already have the relevant helper function
TS: the original case is already contrived
TS: if you want your bound to a different type there is a chance that your increment will never actually hit the bound
TS: I don’t think we need to bend over backwards to accommodate common everywhere
TK: we can make this range random access if it the underlying are random access and sized
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
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
JM: but we don’t know if there is an end
TK: then it is up to the implementation to do it properly: it is always possible to do it
JM: does the upcoming common-range discussion affect the correctness?
TK: yes, if it is a common-range then that is the case
- HX: [op-] We rephrased the whole specification
- [DONE] JG: I think we wanted to have the “let” refer to the sum
JM: the other operator the same: the “for every …” should be after the let
JM: and we need another earmark
- HX: next change [iter_swap]
- [NAD] JM: isn’t there iterator_reference_t instead of iterator_t
DK: the (its, its) is odd
JM: it says afterwards but it unclear what the respectively refers to
JW: the views is the unexpanded pack; if we have better wording use it, otherwise leave it
- [Done] JM: why is there are const at the end?
JW: we normally have west-const

- [Done] JW: based on the chat the short-circuiting in the requires clause doesn’t happen
JW: TS what do you think of TK’s proposal in the chat? This way we could avoid the fold-expression if the first bit fails
21 changes: 11 additions & 10 deletions concat.md
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,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-arg*@<@*maybe-const*@<Const, Fs>>);
```
:::
Expand Down Expand Up @@ -1451,13 +1451,13 @@ 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~*@` be
- [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, of type
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>>...>>`,
equivalent to

Expand Down Expand Up @@ -1490,8 +1490,9 @@ 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_*@)))`.
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`
Let `s` denote the sum of the sizes of all the ranges
`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>>...>>`,
equivalent to

Expand Down Expand Up @@ -1589,13 +1590,13 @@ std::visit(
(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.
where `its` is a pack of lvalues of type `const iterator_t<@*maybe-const*@<Const, Views>>` respectively.

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

```cpp
(swappable_with<iterator_t<@*iterator*@>, iterator_t<@*iterator*@>> && ... &&
indirectly_swappable<iterator_t<@*maybe-const*@<Const, Views>>>)
swappable_with<iter_reference_t<@*iterator*@>, iter_reference_t<@*iterator*@>> &&
(... && indirectly_swappable<iterator_t<@*maybe-const*@<Const, Views>>>)
```

:::
Expand Down
6 changes: 3 additions & 3 deletions impl/concat/concat.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -594,9 +594,9 @@ class concat_view : public view_interface<concat_view<Views...>> {
}

friend constexpr void
iter_swap(const iterator& x, const iterator& y) requires(
swappable_with<iter_reference_t<iterator>, iter_reference_t<iterator>> &&...&&
indirectly_swappable<iterator_t<__maybe_const<Const, Views>>>)
iter_swap(const iterator& x, const iterator& y) requires
swappable_with<iter_reference_t<iterator>, iter_reference_t<iterator>> &&
(...&& indirectly_swappable<iterator_t<__maybe_const<Const, Views>>>)
// todo: noexcept?
{
std::visit(
Expand Down

0 comments on commit a42d0b9

Please sign in to comment.