RFC for zip_view implementation, for oneDPL C++20 #1931
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| # zip_view Support for the oneDPL Range APIs with C++20 | ||
|
|
||
| ## Introduction | ||
| `std::ranges::zip_view` is a powerful utility that enables developers to combine two or more ranges into a single view, | ||
| where each element is represented as a tuple containing corresponding elements from each input range. | ||
|
|
||
| ## Motivations | ||
| `std::ranges::zip_view` is a convenient way to combine multiple ranges into a single view, where each element of | ||
| the resulting range is a tuple containing one element from each of the input ranges. This can be particularly | ||
| useful for iterating over multiple collections in parallel. `std::ranges::zip_view` was introduced in C++23, | ||
| but many developers are still using C++20 standard. So, oneDPL introduces `oneapi::dpl::ranges::zip_view`, | ||
| with the same API and functionality as `std::ranges::zip_view`. | ||
|
Comment on lines
+10
to
+12
There was a problem hiding this comment. Thinking more of If the goal is only to close the gap between C++20 and C++23, while the standard C++23 |
||
|
|
||
| ### Key Requirements | ||
akukanov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| `oneapi::dpl::ranges::zip_view` should be: | ||
| - compilable with C++20 version (minimum) | ||
dmitriy-sobolev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - API-compliant with `std::ranges::zip_view` | ||
| - random accessible view; the "underlying" views also should be random accessible | ||
dmitriy-sobolev marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. This is how cppreference describes the category for
Do we want the same adaptive behavior, or do we want to require all the base ranges to be random access ranges? There was a problem hiding this comment. In my understanding our intention was to provide 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. And we should also think of implications. If our |
||
| - in case of a device usage: a device copyable view if the all "underlying" views are device copyable views | ||
|
|
||
| `oneapi::dpl::ranges::zip_view::iterator` should be: | ||
| - value-swappable (https://en.cppreference.com/w/cpp/named_req/ValueSwappable) | ||
| - implicitly convertible to `oneapi::dpl::zip_iterator` | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. As I mentioned above, I have in my mind at least one practically-viable example: There was a problem hiding this comment. 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. There was a problem hiding this comment. Yes, explicitly conversion is "safer"... There was a problem hiding this comment. Another compatibility/conversion question is with regard to the C++23 There was a problem hiding this comment. Do you mean the standard There was a problem hiding this comment. 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. |
||
| - able to be used with the non-range algorithms | ||
|
|
||
| ### Implementation proposal | ||
| - `oneapi::dpl::ranges::zip_view` is designed as a C++ class which represents a range adaptor (see C++ Range Library). | ||
| This class encapsulates a tuple-like type to keep a combination of two or more ranges. | ||
| - The implementation provides all necessary operators to satisfy 'random accessible view' requirement. | ||
| - To ensure device copyability, `oneapi::dpl::__internal::tuple` is proposed as a tuple-like type for underlying elements. | ||
| - To provide a value-swappable requirement `oneapi::dpl::__internal::tuple` is proposed as a dereferenced value for | ||
| `oneapi::dpl::ranges::zip_view::iterator` due to `std::tuple` not satisfying the value-swappable requirement in C++20. | ||
| - Usage of C++ concepts is desirable to write type requirements for types, methods and members of the class. | ||
| - C++20 is minimum supported version for the class. It allows using modern C++ features such as concepts and others. | ||
|
|
||
| ### Test coverage | ||
|
|
||
| - `oneapi::dpl::ranges::zip_view` is tested itself, base functionality. | ||
|
There was a problem hiding this comment. What amount of testing would be necessary to ensure the API compliance with There was a problem hiding this comment. I think to have at least one call of each method of There was a problem hiding this comment. 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. |
||
| - should be tested with at least one oneDPL range based algorithm. | ||
| - should be tested with at least one oneDPL iterator based algorithm. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this feature going to be experimental? If yes, I guess the namespace should be
oneapi::dpl::experimental::ranges::zip_view.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding to
experimentalnamespace 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,experimentalword 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you expect the feature to be productized fast, similarly to the C++20 range support. Hence, having
experimentalnamespace 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding here was that we were shooting for full product status for the ranges MVP APIs including
zip_viewafter the experience of the existing experimental ranges APIs. We already have an experimentalzip_viewthat has been around for some time now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, indeed, thanks for reminding about
oneapi::dpl::experimental::ranges::zip_view. Then, "supported" stage looks better than "experimental" as the next target.Could you elaborate why you prefer "experimental" stage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, there is an internal utility -
oneapi::dpl::__ranges::zip_viewin using everywhere in the internal oneDPL implementation with C++17 support.Also
oneapi::dpl::__ranges::zip_viewis injected intooneapi::dpl::experimental::rangesnamespace.oneapi::dpl::experimental::ranges::zip_viewis 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_PRESENTmode to have the proposed newzip_viewin theoneapi::dpl::experimental::rangesnamespace and 2) injection of existingoneapi::dpl::__ranges::zip_viewintooneapi::dpl::experimental::rangesotherwise (in case of C++17).1a) the "old"
oneapi::dpl::__ranges::zip_viewhas 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 ofstd::views::elements<0>(__in_rng)IMHO.@akukanov , @danhoeflinger , @dmitriy-sobolev what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_viewhas 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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing zip_view doesn't work with C++ standard ranges. That's why we need new one for _ONEDPL_CPP20_RANGES_PRESENT mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.