RFC for zip_view implementation, for oneDPL C++20

[MikeDvorskiy]

The proposal to implement zip_view in oneDPL. A part of the general RFC discussion #1944 about implementing useful C++23 views.

[dmitriy-sobolev]

There is one question regarding the conversions of the iterators. The rest are minor stylistic and wording corrections.

[dmitriy-sobolev]

Is this feature going to be experimental? If yes, I guess the namespace should be oneapi::dpl::experimental::ranges::zip_view.

[MikeDvorskiy]

Regarding to experimental namespace here. I don't know... It depends on RFC process. This document is a proposal in the product or to be productized - that's a goal. So, experimental word looks weird.

Talking about the next stage of our doing with zip_view, yes we would like to have it in the experimental namespace first.

[dmitriy-sobolev]

I guess you expect the feature to be productized fast, similarly to the C++20 range support. Hence, having experimental namespace will impair user experience due to the changes it involves.

Does it make sense to treat it as an extension straight away with "supported" as the next stage? Do you feel confident regarding the current design?

[danhoeflinger]

My understanding here was that we were shooting for full product status for the ranges MVP APIs including zip_view after the experience of the existing experimental ranges APIs. We already have an experimental zip_view that has been around for some time now.

[dmitriy-sobolev]

Ah, indeed, thanks for reminding about oneapi::dpl::experimental::ranges::zip_view. Then, "supported" stage looks better than "experimental" as the next target.

Talking about the next stage of our doing with zip_view, yes we would like to have it in the experimental namespace first.

Could you elaborate why you prefer "experimental" stage?

[MikeDvorskiy]

Basically, there is an internal utility - oneapi::dpl::__ranges::zip_view in using everywhere in the internal oneDPL implementation with C++17 support.
Also oneapi::dpl::__ranges::zip_view is injected into oneapi::dpl::experimental::ranges namespace.
oneapi::dpl::experimental::ranges::zip_view is used in 4 places in oneDPL impl... and probably in a user'code, because it is a public name.

So, I would propose 1) for _ONEDPL_CPP20_RANGES_PRESENT mode to have the proposed new zip_view in the oneapi::dpl::experimental::ranges namespace and 2) injection of existing oneapi::dpl::__ranges::zip_view into oneapi::dpl::experimental::ranges otherwise (in case of C++17).

1a) the "old" oneapi::dpl::__ranges::zip_view has public "tuple()" method... and it used in the following context:
std::get<0>(__in_rng.tuple()), Actually, it is not correct. Should be an oneDPL internal analog of std::views::elements<0>(__in_rng) IMHO.

@akukanov , @danhoeflinger , @dmitriy-sobolev what do you think?

[danhoeflinger]

In theory, we have license to change "experimental" things, as you suggest (in C++20 in this case).

What we need to be careful of is that the existing zip_view has been stable for a long time, and I think used in non-experimental production code internally. I think we need to confirm and be sure that we are not introducing breaking changes into non-experimental code, and that any breaking changes to the experimental zip_view with C++20 enabled are well described and kept to a minimum.

As far as the RFC is concerned, we need to make it clear here the plan for how it will coexist with what already exists (basically what you describe above).

[MikeDvorskiy]

the existing zip_view has been stable for a long time

The existing zip_view doesn't work with C++ standard ranges. That's why we need new one for _ONEDPL_CPP20_RANGES_PRESENT mode.

[danhoeflinger]

Yes that makes sense. My reason for stating its stability is not to argue that a new version isn't needed, but merely to say that users may have more of an expectation to treat the existing implementation as a "supported" feature rather than an "experimental" feature, because we have basically treated it as such.
Therefore, changes may take them by surprise. We are not doing anything wrong by changing it, as it is experimental, we just need to be aware that it may affect users.

[akukanov]

So, I would propose 1) for _ONEDPL_CPP20_RANGES_PRESENT mode to have the proposed new zip_view in the oneapi::dpl::experimental::ranges namespace and 2) injection of existing oneapi::dpl::__ranges::zip_view into oneapi::dpl::experimental::ranges otherwise (in case of C++17).

If the experimental algos based on non-standard views are available for C++20, this can still be a breaking change I am afraid.

We can go with e.g. oneapi::dpl::ranges::zip_view_experimental, I guess. It's ugly and does not follow the convention, but the intent is clear.

[akukanov]

This is how cppreference describes the category for std::ranges::zip_view:

zip_view always models input_range, and models forward_range, bidirectional_range, random_access_range, or sized_range if all adapted view types model the corresponding concept.

Do we want the same adaptive behavior, or do we want to require all the base ranges to be random access ranges?

[MikeDvorskiy]

In my understanding our intention was to provide zip_view like adaptor (for C++20), which "works (is pipeable)" with C++20 ranges/views and can be passed into oneDPL algorithms, because existing experimental::zip_view "doesn't work (is not pipeable)" with C++20 ranges/views.

As far as https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p3179r2.html tells that only random access ranges are accepted for range based oneDPL algorithm, I think we want to support zip_view only for random_access_range - to require all the base ranges to be random access ranges.

[akukanov]

OK, so there is no intent to make it adaptable to different range categories and usable more widely. It needs to be clear in the documentation & specification.

[akukanov]

And we should also think of implications. If our zip_view cannot be used with ranges that are not random access, could it make developers hesitant to use it, possibly impacting oneDPL usage?

[akukanov]

The same question here: do we require base views to be device copyable, or do we say that the zip_view is device copyable if the base views are?

[MikeDvorskiy]

yes, "a device copyable view itself" - is not clear and the sentence should be rephrased.

Yes, we say that the zip_view is device copyable if the base views are.
The following sentence guarantees that:
"To provide a device copyability requirement oneapi::dpl::__internal::tuple is proposed as tuple-like type underhood."

[akukanov]

OK. Please adjust the text to make the intent clear.

[akukanov]

Do we have in mind some reasonable use cases for this conversion? Because if we do not, maybe we should not add extra complexity and extra risks with the implicit conversion.

[MikeDvorskiy]

As I mentioned above, I have in my mind at least one practically-viable example:
A user has some function like void foo(oneapi::dpl::zip_iterator b, oneapi::dpl::zip_iterator e) and has some ranges combined into one zip_view and is going to call foo with zip_view.

[akukanov]

I tend to think that functions usually take generic iterators rather than specific ones. And as we know, implicit conversions may affect SFINAE, overload resolution sets, etc.

Maybe it is sufficient for the iterator traits to match, including their value and reference types? That makes sense and seems less risky to me. We can consider conversion (implicit or explicit) later, based on real demand and implementation complexity.

[MikeDvorskiy]

Yes, explicitly conversion is "safer"...
I don't mind to consider such conversion support "on demand".

[akukanov]

What amount of testing would be necessary to ensure the API compliance with std::ranges::zip_view, stated as a requirement?

[MikeDvorskiy]

I think to have at least one call of each method of oneapi::dpl::ranges::zip_view with result checking is quite enough.
As an additional test coverage we may consider to take LLVM tests for std::ranges::zip_view, if it is technically possible by C++23/C++20 reasons.

[MikeDvorskiy]

Talking about "the API compliance with std::ranges::zip_view". I mean the public methods and types are equivalent at least for underlying random sized ranges: ones are callable and result the same. And also pipeable feature is supported.

[akukanov]

I approve the proposal for implementation as an experimental feature.

The questions currently open in the review can be addressed later; just keep track of them please (for example, add to a special section at the end of the document).

[dmitriy-sobolev]

The RFC also looks good to me.

My additional recommendation is to express the criteria for the feature to become fully supported. I believe, it can be done at a later stage, e.g. when the experimental feature is implemented or thoroughly reviewed for a better grasp of the context.

[akukanov]

Thinking more of zip_view, I now wonder if we should add it to the specification or rather should provide it as a library extension.

If the goal is only to close the gap between C++20 and C++23, while the standard C++23 zip_view is well usable with oneDPL, including with device policies - then maybe it is not worth adding our zip_view variation to the specification. Perhaps especially so if it cannot be used as a substitution for the standard zip_view in all scenarios (that means, if developers cannot always use the oneDPL variation and may need to choose between that and the standard one).

[akukanov]

Another compatibility/conversion question is with regard to the C++23 zip_view. If our view cannot always be used in place of the standard one, perhaps we should consider how developers would switch between the two.

[MikeDvorskiy]

Do you mean the standard zip_view C++23 might be not device copyable f.e.? And how developers can use our view instead of?

[akukanov]

No, I meant if our view is not going to support all requirements/use cases of the standard (e.g. not going to work with bidirectional etc. views), then developers cannot use it all the time. So it makes sense to think if there could be cases to transit from one zip view to another.