huixie90 / cpp_papers

Other
5 stars 3 forks source link

`concat_view::end()` should be more constrained in order to support non-copyable iterators #10

Open yaito3014 opened 2 weeks ago

yaito3014 commented 2 weeks ago

I found a possible issue in concat_view's implementation.

There is a case that concat(a, b) compiles but concat(b, a) does not.

auto range_copyable_it = std::vector<int>{1, 2, 3};

std::stringstream ss{"4 5 6"};
auto range_noncopyable_it = std::views::istream<int>(ss);

auto view1 = std::views::concat(range_copyable_it, range_noncopyable_it);
static_assert(std::ranges::range<decltype(view1)>);               // ok
assert(std::ranges::equal(view1, std::vector{1, 2, 3, 4, 5, 6})); // ok

auto view2 = std::views::concat(range_noncopyable_it, range_copyable_it);
// static_assert(std::ranges::range<decltype(view2)>);               // error
// assert(std::ranges::equal(view2, std::vector{4, 5, 6, 1, 2, 3})); // error

The reason behind this is as follows:

Firstly, if all Views... satisfy the std::ranges::range concept, then concat_view should also satisfy it. However, if any of the Views... have a non-copyable iterator and the last view is common_range, the proposed concat_view fails to model a range.

For concat_view to model a range, its sentinel must satisfy std::semiregular, but concat_view::end() returns a concat_view::iterator, which is non-copyable if the underlying iterator is non-copyable. This issue arises from the proposed implementation, where the iterator uses std::variant. Although this specification is exposition-only, even if an alternative type-erasure mechanism is used, copying is still required if the user attempts to copy an iterator.

To resolve the issue, concat_view::end() can fallback to returning std::default_sentinel in such cases.

// before
if constexpr (common_range<maybe-const<is-const, Views...[N - 1]>>) {
// after
if constexpr ((semiregular<iterator_t<Views>> && ...) && common_range<maybe-const<is-const, Views...[N - 1]>>) {

Unfortunately, as a side effect, this fix would prevent concat_view from being a common_range in certain situations. According to P2542R8:

concat_view can be common_range if the last underlying range models common_range

However, this is no longer true after applying my fix. On the other hand, the current draft (N4988) does not mention when concat_view can model common_range. I assume this omission was for simplicity, but I think the new behavior should be documented as a side note.

That said, these two issues cannot be resolved simultaneously due to implementability. Therefore, I suggest applying the fix regardless and accepting that concat_view will not always inherit common_range.

I think this issue should be re-evaluated by LEWG or LWG before C++26 is shipped. Could I ask you to review the problem as the author?

huixie90 commented 2 weeks ago

Hello, @yaito3014 , thanks for reporting the issue. Could you please submit an LWG issue? For bug fixes, we don’t have to go through LEWG.

https://cplusplus.github.io/LWG/lwg-active.html#submit_issue The email address for submitting issue is on the top of that page

the issue will be then discussed in the reflector and if enough people voted P0, potentially it be tentatively ready for next committee meeting

yaito3014 commented 1 week ago

Thank you for response. I'm currently writing an email. 🚀

P.S. I have submitted the issue to the list.