[oneDPL][ranges] + zip_view implementation for C++20

[MikeDvorskiy]

[oneDPL][ranges] + zip_view implementation for C++20. The implementation and test.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2321r2.html

• namespace: oneapi::dpl::ranges
• based on oneDPL device copyable tuple
• LLVM tests added

An update:

• rebased on the current main
• fixed the errors within test_dangling_pointers_args_<x> and the other tests.

oneapi::dpl::experimental::ranges::zip_view
oneapi::dpl::experimental::ranges::views::zip
oneapi::dpl::experimental::views::zip

#if _ONEDPL_CPP20_RANGES_PRESENT
using oneapi::dpl::ranges::__internal::zip_view;
#else
using oneapi::dpl::__ranges::zip_view;
#endif //_ONEDPL_CPP20_RANGES_PRESENT

[dmitriy-sobolev]

Let me leave just a couple of minor comments. I am going to look at the whole PR during this week.

[dmitriy-sobolev]

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 (*move construction is not checked)
  • another zip_view
• ranges::zip(...) 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).

Update. I see that the PR integrates the whole LLVM test suite now. The table is not relevant now

[dmitriy-sobolev]

The description says:

... based on oneDPL device copyable tuple

According to SYCL 2020, std::tuple is also device copyable (if the wrapped types are also device copyable, of course). Does it make sense to use std::tuple instead of the internal one?

[danhoeflinger]

The description says:

... based on oneDPL device copyable tuple

According to SYCL 2020, std::tuple is also device copyable (if the wrapped types are also device copyable, of course). Does it make sense to use std::tuple instead of the internal one?

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). std::tuple isn't specified to be trivially copyable.

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 std::tuple in some ways but I think this is the better option.

[MikeDvorskiy]

to use std::tuple instead of the internal one?

Actually, before C++23 standard library there is a problem with std::tuple swap ability. It is bigger issue that doesn't allow to use std::tuple for zip_view implementation for oneDPL C++20.

[MikeDvorskiy]

regarding > * cbegin + cend
cppreference:

cbegin
  
(C++23)
 
returns a constant iterator to the beginning of the range.
(public member function of std::ranges::view_interface<D>)
cend
  
(C++23)
 
returns a sentinel for the constant iterator of the range.
(public member function of std::ranges::view_interface<D>)

[kboyarinov]

Why cannot we use x.current <=> y.current? Spaceship operator is undefined for some of our internal iterator types or it is undefined for internal tuple?

[MikeDvorskiy]

yes, spaceship operator is undefined for x.current_...

[danhoeflinger]

When adding comparison between internal tuples, we looked at adding spaceship and it was deemed not worth the effort at the time. We could use this as motivation for adding it, but it would require a bit of work. We would need to trigger it off of __cpp_lib_three_way_comparison >= 201907L, and in that case replace the existing comparison operators with the spaceship, to time it properly with std::tuple <=> operator

#1472 (comment)

[danhoeflinger]

The thought was that it wasn't worth the maintenance because we would need to implement both to support our full support matrix, and it would be only helpful for those explicitly calling <=> on the internal tuple (which is what we might want to do here as I understand it).

[MikeDvorskiy]

In any case it doesnt block zip_view productization, I believe. And we keep a spaceship operator for zip_view as is proposed.

[dmitriy-sobolev]

Our testing framework is not adapted for assert: it defines NDEBUG during release test builds. We should either avoid that definition, or do not rely on assert in the tests.

[MikeDvorskiy]

1. In spite of that we are using assert in our tests more then 10 files... It seems an issue should be created.
2. I know that a better way to use our own macro like EXPECT_TRUE f.e.

[dmitriy-sobolev]

1. I will create it.
2. That sounds good. Could you do it? I see that there is still one assert in the test.

[dmitriy-sobolev]

1. I will create it.

Done: #1945.

[MikeDvorskiy]

2. done.
   And I suggest offline discussion about assert / EXPECT_TRUE / NDEBUG

[dmitriy-sobolev]

regarding > * cbegin + cend cppreference:

cbegin
  
(C++23)
 
returns a constant iterator to the beginning of the range.
(public member function of std::ranges::view_interface<D>)
cend
  
(C++23)
 
returns a sentinel for the constant iterator of the range.
(public member function of std::ranges::view_interface<D>)

zip_view is based on view_iterface, and the later does not have cbegin/cend in c++20. That means our c++20 zip_view will not have cbegin/cend, and we are not going to provide them. Is that correct understanding?

[kboyarinov]

Did not we think about having oneapi::dpl::zip_view as part of namespace std::ranges prior to C++23?
It will allow the user to have portable code for each version of the standard. Otherwise, we will need to think about aligning oneapi::dpl::zip_view with the C++23 and later as well.

[MikeDvorskiy]

Do you mean a kind of name injection into std::ranges prior to C++23? Good question.

On one hand - it is useful when a user is going to modify his code forward to C++23.
On the other hand, having std::ranges::zip_view (in result of name oneapi::dpl::zip_view injection), enforces us to be fully compliance with standard zip_view C++23. At least, the proposed zip_view implementation works only with random access views, for example. And we don't need more for oneDPL needs.

I would suggest rather to have an alias oneapi::dpl::zip_view, which is our own implementation class prior to C++23 and which is std::ranges::zip_view for C++23 and higher.

[kboyarinov]

I like the idea of having the alias to std::ranges::zip_view in oneDPL for C++23 and higher. It perfectly fits the idea of having everything that should work in the offloaded code in oneapi::dpl namespace.

[akukanov]

We can consider such an alias, but for me that would mean that our implementation of zip_view should support (almost) all of what the standard requires. And since zip_view is based on the standard tuple, which in C++20 has some limitations for our needs, I am not sure to which extent the compliance with the standard is feasible.

Also there still will be a question whether it should be a specified feature or an extension.

[Copilot]

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Inconsistent indentation found: Lines 261-265 use tabs for indentation while the rest of the file uses spaces. This violates the consistent code style used throughout the file.

[danhoeflinger]

I really don't like this construction. A static member function of iterator whose only purpose is to reach into sentinel to get __end, with a friend declaration in sentinel class.

Can we just make iterator a friend of sentinel and reach directly to get __end? or add a protected member function if you want to have some function instead of using the member field directly.

If you do add a protected member function, I'd reconsider the name to something like __y.__get_tuple_of_sentinels() or __y.__get_end() . "current" implies that the sentinel has a current state, which is somewhat against the idea if I'm not mistaken.

(Also as mentioned in the copilot review, there are some tabs in the code related to this that need to be replaced with spaces.)

[MikeDvorskiy]

I really don't like this construction

@danhoeflinger , any other approaches don't work for the all compilers (especially microsoft) compiler. I tried it... You may try doing also several another approaches, if you want.

Actually we have tricky cases, when we have to support arithmetic between two different C++ classes - iterator and sentinel.
Moreover, I didn't invent the solution with __get_current - C++ 23 standard library does it such way. I have decided to use the same approach. This approach has the minimum of the mutual access between iterator and sentinel, comparing with another approaches which are compiled with the all compilers.

Only one thing that I missed - to try placing iterator::__get_current in a private section of iterator class...

P.S.

            template <bool _IteratorConst>
            _NODISCARD static constexpr const auto& _Get_iterator_tuple(
                const _Iterator<_IteratorConst>& _Itr) noexcept {
                // NOTE: This function is necessary because friend functions are never
                // member functions, and friendship is not transitive.
                return _Itr._Curren

https://github.com/microsoft/STL/blob/main/stl/inc/ranges#L7416

[danhoeflinger]

I'll do some experiments in godbolt. Perhaps this is the best way but it seems strange from my perspective.

[danhoeflinger]

Suggested change

	template <bool C, typename... _Views>
	concept __all_bidirectional = (std::ranges::bidirectional_range<__maybe_const<C, _Views>> && ...);
	template <bool C, typename... _Views>
	concept __all_random_access = (std::ranges::random_access_range<__maybe_const<C, _Views>> && ...);
	template <typename... Rs>
	concept __zip_is_common = (sizeof...(Rs) == 1 && (std::ranges::common_range<Rs> && ...)) ||
	(!(std::ranges::bidirectional_range<Rs> && ...) && (std::ranges::common_range<Rs> && ...)) ||
	((std::ranges::random_access_range<Rs> && ...) && (std::ranges::sized_range<Rs> && ...));
	// Equality semantics differ by iterator strength:
	// - bidirectional-or-stronger: tuple equality ("all components equal")
	// - otherwise: zip termination equality ("any component equal")
	template <bool C, typename... _Views>
	concept __all_bidirectional = (std::ranges::bidirectional_range<__maybe_const<C, _Views>> && ...);
	template <bool C, typename... _Views>
	concept __all_random_access = (std::ranges::random_access_range<__maybe_const<C, _Views>> && ...);
	// zip_view can model common_range only when end() can be represented as an iterator
	// with correct "shortest range wins" semantics; when false, return a sentinel from end()
	//
	// Cases:
	// 1) Single view: commonness is inherited from that view.
	// 2) Not all bidirectional + all common: iterator==iterator for this zip iterator uses
	// "any component equal", which matches zip termination.
	// 3) All random_access + all sized: we can synthesize a canonical iterator end as
	// begin() + min(size...), so iterator end is well-defined even for multiple ranges.
	template <typename... Rs>
	concept __zip_is_common = (sizeof...(Rs) == 1 && (std::ranges::common_range<Rs> && ...)) ||
	(!(std::ranges::bidirectional_range<Rs> && ...) && (std::ranges::common_range<Rs> && ...)) ||
	((std::ranges::random_access_range<Rs> && ...) && (std::ranges::sized_range<Rs> && ...));

Some comments like this (and similar elsewhere for code involving the changing == semantics and end() return) would be very helpful in understanding this.

[MikeDvorskiy]

I don't know...

To me the native C++ code is more readable and understandable then so long text description.
But if it helps somebody else, I don't mind.
But I guess it should be in the well known terms of C++standard. For example, I don't understand meaning of "we can synthesize a canonical iterator....". I can guess, but I don't want to do it - I prefer read a C++ code instead.

[danhoeflinger]

I suppose I'm not as familiar with the std zip view coming into this review.
The biggest confusion for me was changing equality operator based upon if all ranges were bidirectional, and then how that impacted end() and this __zip_is_common implementation. If we can describe that in this general area, I'd be satisfied. I agree that the proposed comments are probably too verbose.

[MikeDvorskiy]

About bidirectional and end(). It was designed intentionally.
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2321r2.html

[danhoeflinger]

Yes, I understand all that now and it makes sense. However, just reading the code without that prior knowledge this was difficult to understand. This is why I think some comments would be nice. A comment could even link to https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2321r2.html#when-is-zip_view-a-common_range for instance.

[kboyarinov]

Should this function be static?
__apply_to_tuple function is not static and calling static function from inline is UB by standard, that currently works as expected.

[MikeDvorskiy]

I don't remember a reason why a static is written here.. Probably, the function was defined in a class scope in the past. But now it is a global function and we can remove "static".