-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[oneDPL][ranges] + zip_view implementation for C++20 #1877
base: main
Are you sure you want to change the base?
Conversation
…ew::iterator type
…d::apply doesnt work with oneDPL tuple
…d::apply doesnt work with oneDPL tuple
…de duplication" This reverts commit fc04caf.
9342293
to
91cf6b0
Compare
603daed
to
80ef9c5
Compare
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.
Let me leave just a couple of minor comments. I am going to look at the whole PR during this week.
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.
What do you think about testing the following properties of zip_view/zip?
- begin + end (they should also probably be checked if they are iterators)
- cbegin + cend
- front + back
- size
- empty
- operator bool
- construction:
- ranges::zip(...) + ranges::zip_view(...)
- empty view (views::zip())
- another view (also check if it does move construction)
- another zip_view
- is a customization point object
I assume that the functionality must match what is required from c++ standard looking at the description. That's why I suggest testing all these properties. Let me know if the implementation is expected to be more relaxed (and how if it is).
The description says:
According to SYCL 2020, |
The advantage here of using oneDPL's tuple is that it is trivially copyable (for trivially copyable types) which means that any class or structure which uses oneDPL's tuple can be implicitly device copyable rather than requiring a specialization of sycl::is_device_copyable (because of the tuple). The advantage is not just to avoid extra code here but also to not impose requirements on users to provide these specializations for types which are composed of a zip_view as well. There can be downsides to using oneDPLs tuple in that it may not be as fully featured as |
Actually, before C++23 standard library there is a problem with |
static decltype(auto) | ||
apply_to_tuple_impl(_ReturnAdapter __tr, _F __f, _Tuple& __t, std::index_sequence<_Ip...>) | ||
{ | ||
return __tr(__f(oneapi::dpl::__internal::get_impl<_Ip>()(__t))...); |
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.
Does it makes sense to use std::get
here instead? Would not it be more readable?
static auto | ||
apply_to_tuple(_F __f, _Tuple& __t) | ||
{ | ||
apply_to_tuple_impl([](auto...){}, __f, __t, std::make_index_sequence<sizeof...(Views)>{}); |
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.
apply_to_tuple_impl([](auto...){}, __f, __t, std::make_index_sequence<sizeof...(Views)>{}); | |
apply_to_tuple([](auto...){}, __f, __t); |
} | ||
|
||
template <typename _F, typename _Tuple> | ||
static auto |
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.
static auto | |
static decltype(auto) |
|
||
public: | ||
zip_view() = default; | ||
constexpr zip_view(Views... views) : views_(views...) {} |
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.
constexpr zip_view(Views... views) : views_(views...) {} | |
constexpr zip_view(Views... views) : views_(std::move(views)...) {} |
static decltype(auto) | ||
apply_to_tuple(_ReturnAdapter __tr, _F __f, _Tuple& __t) | ||
{ | ||
return apply_to_tuple_impl(__tr, __f, __t, std::make_index_sequence<sizeof...(Views)>{}); |
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.
Does it makes sense to move functions to the apply_to_tuple_impl
?
|
||
friend constexpr iterator operator+(iterator it, difference_type n) | ||
{ | ||
return it+=n; |
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.
return it+=n; | |
return it += n; |
|
||
friend constexpr iterator operator+(difference_type n, iterator it) | ||
{ | ||
return it+=n; |
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.
return it+=n; | |
return it += n; |
|
||
friend constexpr iterator operator-(iterator it, difference_type n) | ||
{ | ||
return it-=n; |
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.
return it-=n; | |
return it -= n; |
end_type end_; | ||
}; // class sentinel | ||
|
||
constexpr auto begin() requires (std::ranges::range<Views> && ...) // !simple_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.
Please add a TODO comment or add a simple view constraint everywhere.
c17532a
to
a9c5e7c
Compare
a9c5e7c
to
ad4e8ac
Compare
[oneDPL][ranges] + zip_view implementation for C++20. The implementation and test.