Remove owning_view

[akukanov]

@benbrock @lslusarczyk - this patch proposes to completely remove the custom implementation of owning_view.

For the context, that implementation is used as the return type for views::zip::segments() - it wraps a vector of views paired with a rank that the function builds. No other use of owning_view exists in the proposed oneDPL patch, but in the standalone DR repo it is also used for the same purpose by distributed_dense_matrix::segments().

A comment in detail/owning_view.hpp says that the custom implementation of owning_view there is needed only for ranges-v3. However, when I tried to replace it to std::ranges::owning_view, it resulted in compilation errors, for example (many details omitted):

/home/runner/work/oneDPL/oneDPL/test/distributed_ranges/sp/../include/common_tests.hpp:214:15:
error: no matching function for call to object of type 'const _Join'
  214 |   auto flat = stdrng::views::join(segments);
      |               ^~~~~~~~~~~~~~~~~~~
...
/usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/ranges:3224:2:
note: candidate template ignored: constraints not satisfied
[with _Range = stdrng::owning_view<vector<zip_view<span<unsigned int, _Iterator>, remote_span<int, device_ptr<int>>>>> &]
 3224 |         operator() [[nodiscard]] (_Range&& __r) const
      |         ^
/usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/ranges:3221:16:
note: because 'std::ranges::owning_view< ... > &' does not satisfy 'viewable_range'
 3221 |       template<viewable_range _Range>
      |                ^
/usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/ranges_base.h:809:41:
note: because 'constructible_from<remove_cvref_t<owning_view< ... > &>, std::ranges::owning_view< ... > &>'
evaluated to false
  809 |       && ((view<remove_cvref_t<_Tp>> && constructible_from<remove_cvref_t<_Tp>, _Tp>)
      |                                         ^
...

The error happens because std::ranges::owning_view is not copyable: see https://en.cppreference.com/w/cpp/ranges/owning_view#ctor. Therefore an lvalue of owning_view is not a viewable_range (see the above log) and cannot be used as an argument to construct std::views::all as well as other views (as far as I can see, most views rely on all at least for CTAD).

The custom implementation of owning_view in DR does not delete the copy constructor and therefore has no such problem, but consequently it is not compliant to the standard definition of owning_view.

So, if the custom owning_view in DR is replaced with the standard one, then storing the result of ranges::segments() into a variable and then building a view over that variable (as in the test code below) does not work:

  auto segments = dr::ranges::segments(r);
  auto flat = stdrng::views::join(segments); // the compilation error shown in the log above

However it is not quite clear to me why views::zip::segments() wrap the vector it builds into owning_view before returning. I guess the idea could have been that ranges::segments() applied to a view must return a view as well. or maybe you wanted ranges::segments() to be directly used in a view pipe. As far as I can tell, however, the method segments() of your distributed containers returns a copy of their vector of segments. So, unless I miss something, returning a vector of segments without wrapping it into owning_view should seemingly be just fine.

[lslusarczyk]

Both DR tests timed out on "Zip/0.Drop" on this change. It needs to be checked if code does not hang now in this case.

[akukanov]

I will take a look at the test. Yet, I would like to have some pros & cons discussion as well, as I might easily miss some reasons for owning_view to really be the proper way to return the segments.

[lslusarczyk]

I will take a look at the test. Yet, I would like to have some pros & cons discussion as well, as I might easily miss some reasons for owning_view to really be the proper way to return the segments.

Sure. I have to look and @BenBrock too. It would be great to simplify the code and remove not needed parts. I had no enough time to rereview owning-view so I only managed to spot this failing test.

[BenBrock]

The issue here is that we have some functions that create views of segments. They take in a range of segments and return a modified view of those segments (e.g. take_segments on line 33 of segments_tools.hpp).

These functions are implemented, naturally, using standard range adaptors like enumerate, take, and transform. Those range adaptors must store a view, which they create using views::all (the stored view being of type views::all_t). If you pass in an lvalue to an owning range (like a vector of device_spans), it will create a ref_view, assuming that the owning range will outlive the lifetime of the newly created ref_view.

I assume this is what's causing problems in the PR---we assume that segments(my_zip_view) returns a view and use an lvalue reference to it to create more views. This can be fixed by either ensuring segments() always returns a view (my preference), or by modifying the code to never use an lvalue reference to the return of segments() (somewhat trickier, and the user could potentially always violate this).

My preference would probably be to simplify using std::views::owning_view. I don't think we have significant uses of copying segment views. If I remember correctly I primarily added a copy constructor because mhp needed it.

[akukanov]

Ben's response gave me the clue for where to look. Apparently, the implementation of take_ & drop_segments does not properly handle rvalue arguments. Simply adding std::forward fixed the tests.

The semantical questions remain though - what should users expect from ranges::segments(dr)? Does it return a view, a range of views, or a range of ranges (assuming that a view is a range, but not vice versa)? Is the returned type copyable, movable but not copyable, or it depends on the range?