RFC for zip_view implementation, for oneDPL C++20 #1931
Conversation
akukanov
requested review from
rarutyun,
dmitriy-sobolev,
danhoeflinger and
adamfidel
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
There was a problem hiding this comment.
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.
MikeDvorskiy
dismissed stale reviews from dmitriy-sobolev and akukanov
via
1f7b132
| 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 |
There was a problem hiding this comment.
When it comes to the specification, this will need to be described as an unspecified tuple which satisfies some set of requirements, unless we want to make the move to specify our internal tuple and make it public (which I doubt we want to do).
There was a problem hiding this comment.
In the context of this, I was looking at the specification for our current zip_iterator, and noticed a couple of discrepancies as compared to our implementation with regard to std::tuple vs our unspecified internal tuple type. I believe that our implementation is intentional and the "correct" thing here and the spec should be changed, because of issues with device copyability when composing types from zip_iterators. std::tuple is not trivially copyable but our internal tuple is.
There was a problem hiding this comment.
We are moving towards specifying the tuple-like type better, most likely as a type that satisfies certain concepts / requirements. That should be done in an new proposal though.
There was a problem hiding this comment.
Yes.. I've mentioned it in the RFC document update.
| - compilable with C++20 version (minimum) | ||
| - API-compliant with `std::ranges::zip_view` | ||
| - in case of a device usage: a device copyable view if the all "underlying" views are device copyable views. | ||
| - To provide a transitive device copyability oneDPL tuple-like type underhood is proposed - `oneapi::dpl::__internal::tuple`. |
There was a problem hiding this comment.
I think which tuple to use is an implementation detail. What we really want is that our zip_view is trivially copyable if all the underlying views are - this is the way to have transitive device copyability for view pipelines.
There was a problem hiding this comment.
Yes, I think "to have transitive device copyability for view pipelines" - very strong argument to want that our zip_view is trivially copyable.
BTW, the proposed implementation has such property. The relevant test also was added.
| - `oneapi::dpl::ranges::zip_view` is based on oneDPL tuple-like type oneapi::dpl::__internal::tuple instead of std::tuple. | ||
| - `oneapi::dpl::ranges::zip_view::iterator::value_type` is oneDPL tuple-like type oneapi::dpl::__internal::tuple instead of std::tuple. |
There was a problem hiding this comment.
I think here we mean that the API of zip_view and its iterator may use a oneDPL tuple-like type instead of std::tuple.
The details like oneapi::dpl::__internal::tuple are already mentioned in the implementation section below.
| - To provide a transitive device copyability oneDPL tuple-like type underhood is proposed - `oneapi::dpl::__internal::tuple`. | ||
|
|
||
| `oneapi::dpl::ranges::zip_view::iterator` should be: | ||
| - value-swappable (https://en.cppreference.com/w/cpp/named_req/ValueSwappable) |
There was a problem hiding this comment.
Perhaps we should explicitly add indirectly_writable as well.
Co-authored-by: Alexey Kukanov <alexey.kukanov@intel.com>
The proposal to implement
zip_viewin oneDPL. A part of the general RFC discussion #1944 about implementing useful C++23 views.