RFC for zip_view implementation, for oneDPL C++20 #1931
+45
−0
Update rfcs/proposed/zip_view/README.md
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
commit 9ab570b9f6c9280a4eefad02798a402acae73684
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.