Skip to content

Commit

Permalink
moved all-xxx-range from zip to utils
Browse files Browse the repository at this point in the history
  • Loading branch information
huixie90 committed Jun 6, 2023
1 parent b284a3b commit b2ea25d
Show file tree
Hide file tree
Showing 2 changed files with 125 additions and 139 deletions.
70 changes: 40 additions & 30 deletions concat.lwg.feedback.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,46 @@
- [Done] we should spell out what reference is in 2.1
- [Done] mattermost: CC: we should ask the aurthors to remove the section numbers (replaced "27.4" with "?.?")
- [Done] (EXTRA TODO) Use std::print instead of std::cout in the example.
- [Done] for the op++ and the others we should use the random-access from zip_view
- Hui: zip_view's op++ requires all-random-access but we need random_access + sized
- Levent: Right. Also, do they mean "op+"?
- Hui: moved `all-random-access` to [range.adaptor.helpers] and reuse
- [Done] the concepts from zip_view [rang.zip.iterator] need to move to a different section: move to [range.utility.helper]
- Hui: I think they refer to all-random-access, all-bidirectional and all-forward. In practice, we can only use all-forward. Not sure if it is worth
- Levent: Agree. Also Casey says "this is a change which can be done outside this paper", anyway.
- Hui: Moved
- [Done] (1.3) use all_forward
- Hui: We could. but it seems to be more effort to reuse this simple concepts than it saves. what do you think?
- Levent: I am mildly for it: It is good for a standard-wide glue. I suggest we walk the extra (micro-)mile.
- [Done] the things which use the general concepts are replaced: op++, op<, etc.
- Hui: Not sure if I understand this. I guess they could either mean
- 1. we could replace `random_access_range<maybe-const<Const, Views>>&&...` with `all-random-access`, or
- 2. some of our ops requires `concat-random-access` but others requires `random_access_range<maybe-const<Const, Views>>&&...`. We should be consistent?
- Levent: For 2, our use isn't accidental: operator< etc can be implemented
without size. So, for those, we can use `all-random-access`. Which is also
what TK was saying (only the "relational operators"). I think we can also
update `concat-random-access`'s def'n to use `all-random-access` for bonus.

# Todos
- [Done] we should use a single definition for the relational operators similar to basic_const_iterator [const.iterator.ops] p19
- Hui: I just learned we can specify that way. Not sure if you like it?
- Levent: Love it. (https://eel.is/c++draft/const.iterators.ops#17 right?)

# Nothing to do

- [Nothing to do] that "expect" needs to be "except"
- Hui: could not find any expect
- Levent: I checked earliest revisions. No "expect" indeed. Not sure what this was.


# Random stuff
- Hui: finally there is a compiler that can run our example (sadly only with std::cout) https://godbolt.org/z/e43r5TbKq

# Todos

- [TODO] mattermost: TZ/CC: wording the concat-bidirectional
- Hui: From the chat history they don't seem to be happy about our wording and had some alternative wordings in the chat
- Levent: Leave this to next round?
- I am bit confused with "Model" vs "Model and Satisfy"...

- [TODO] when doing zip_view and cartesian_product we added maybe_const so we don't need to do that everywhere and we should do that here, too
- Hui: ?
Expand All @@ -44,37 +77,13 @@
becoming a standard exposition symbol (https://eel.is/c++draft/ranges in
synopsis) after zip and cartesian_product was in. Wants to make sure we
don't define it again. (But we don't. So...)


- [TODO] that "expect" needs to be "except"
- Hui: could not find any expect
- Levent: I checked earliest revisions. No "expect" indeed. Not sure what this was.

- [TODO] for the op++ and the others we should use the random-access from zip_view
- Hui: zip_view's op++ requires all-random-access but we need random_access + sized
- Levent: Right. Also, do they mean "op+"?

- [TODO] the concepts from zip_view [rang.zip.iterator] need to move to a different section: move to [range.utility.helper]
- Hui: I think they refer to all-random-access, all-bidirectional and all-forward. In practice, we can only use all-forward. Not sure if it is worth
- Levent: Agree. Also Casey says "this is a change which can be done outside this paper", anyway.

- [TODO] the things which use the general concepts are replaced: op++, op<, etc.
- Hui: Not sure if I understand this. I guess they could either mean
- 1. we could replace `random_access_range<maybe-const<Const, Views>>&&...` with `all-random-access`, or
- 2. some of our ops requires `concat-random-access` but others requires `random_access_range<maybe-const<Const, Views>>&&...`. We should be consistent?
- Levent: For 2, our use isn't accidental: operator< etc can be implemented
without size. So, for those, we can use `all-random-access`. Which is also
what TK was saying (only the "relational operators"). I think we can also
update `concat-random-access`'s def'n to use `all-random-access` for bonus.

- [TODO] (1.3) use all_forward
- Hui: We could. but it seems to be more effort to reuse this simple concepts than it saves. what do you think?
- Levent: I am mildly for it: It is good for a standard-wide glue. I suggest we walk the extra (micro-)mile.
- Hui: Maybe they meant that we should add `Const` as template parameter for concepts like `concat-random-access` and `concat-bidirectional`, like what they did for `all-random-access` etc. So that we don't repeat the `maybe_const` in the every usage of `concat-random-access`? What do you think?

- [TODO] do we want to avoid emplacing ranges we don't need to use?
- Hui: Good point. I think this refers to `advance_fwd` and `advance_bwd`. Maybe we can do something smarter.
- Levent: Oof. Sure, empty subranges. Let the compiler authors do the optim?
(not sure how refined these "equivalent to:" snippets should be)
- Hui: at the moment, if we do `i + n`, if `n` is large enough to skip several ranges in between, we currently specify that it needs to be emplaced for every single begin iterators of the ranges in between, which could be skipped. Not sure how we can do this with an elegant way

- [TODO] explain why this is over-constrained
- Hui: Not sure what this refers to
Expand All @@ -84,18 +93,19 @@
`concat-bidirectional`. But I think we thought about this and decided that
we need the last one sized also. I didn't remember then, and for sure don't
remember now :P
- Hui: Indeed I think the last range does not need to be sized with careful implementation. If you look at the implementation (and the spec) for `x - y`, we calculate all the sizes. But the last value is not used. We could avoid it by carefully doing the bound checking, which is a bit more complicated. In addition, in `x - default_sentinel`, we have to use the last range's size if it is not `common_range`. But this operation (`x - default_sentinel`) is not required for `random_access`

- [TODO] put the front matter after the iterator_category
- Hui: I think this refers to all the preconditions about variant's valueless state. It is good idea but I don't know how to do it. I mean, how to specify what operations are allowed and what are not. Need prio examples.
- Levent: We don't have any operations that are allowed. (no?)
- Hui: Well the generated operations like the destructor and the assignment operators should be allowed. (no?)

- [TODO] we should use a single definition for the relational operators similar to basic_const_iterator [const.iterator.ops] p19
- Hui: I just learned we can specify that way. Not sure if you like it?
- Levent: Love it. (https://eel.is/c++draft/const.iterators.ops#17 right?)

- [TODO] I think the authors need to think about iter_swap and provide rational
- Hui: sg9?
- Levent: Let's just remove it.
- Hui: I thought Tim brought another issue that even the default behaviour (without our specialisation) of `iter_swap` is still problematic for `concat(strings, string_views)`.
https://godbolt.org/z/49esvec96

# Full transcript

Expand Down
Loading

0 comments on commit b2ea25d

Please sign in to comment.