[libcxx-commits] [PATCH] D123466: [libcxx][ranges] add views::join adaptor object. added test coverage to join_view

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision.
var-const added a comment.
This revision is now accepted and ready to land.

LGTM (with just a couple of really small comments). Thanks!



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp:69
+    auto jv = std::views::join(nested);
+    static_assert(std::is_same_v<std::ranges::range_reference_t<decltype(jv)>, Foo(&)[3]>);
+
----------------
huixie90 wrote:
> var-const wrote:
> > var-const wrote:
> > > Question: why not use `same_as` like above?
> > Seems not done, unless I'm missing something.
> Could you please be more explicitly about which line is missing? I think I changed all the things to 
> ```
> std::same_as<expected_type> decltype(auto) x = ...;
> ```
> 
> Line 72 and 75 are not checking the type of the variable but the range_reference_t of the type of the variable.
Ah, I missed that the type is adjusted in this case, thanks for pointing it out.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp:61
     assert(jv.end() == std::ranges::next(jv.begin(), 16));
+    assert(std::as_const(jv).end() == std::ranges::next(std::as_const(jv).begin(), 16));
 
----------------
huixie90 wrote:
> var-const wrote:
> > huixie90 wrote:
> > > var-const wrote:
> > > > Is it possible to check a) whether an iterator or a sentinel is returned; b) whether the returned iterator/sentinel is const?
> > > unfortunately `iterator` and `sentinel` are private exposition only classes. It is not directly accessible. Therefore these tests are testing the type of them indirectly.
> > > 
> > > For example, to test `end` returns an `iterator`, we can check
> > > `std::ranges::common_range<decltype(jv)>` is `true`, because `common_range` means `begin` and `end` returns the same type.
> > > 
> > > if the above is false, it means we have an `sentinel`, to test `end()` returns `sentinel<false>` as oppose to `sentinel<true>`, we can check `!std::same_as<decltype(jv.end()), decltype(std::as_const(jv).end())>` 
> > Optional: maybe create helper type traits or concepts to make this more obvious? E.g. `is_sentinel`, `is_const_iterator`, etc. Feel free to ignore.
> thanks for the suggestion, I did consider to come up with better helper concepts but it is not trivial to come up with one. The concept cannot simple take just one iterator type like
> ```
> template <class T>
> concept is_sentinel = ...
> ```
> because there is no easy way to test if a type is the sentinel type. it will probably end of with something like
> ```
> template <class T, class Iterator>
> concept is_sentinel = !same_as<T, Iterator>
> ```
> which checks if T is the same type as the iterator. but then this concept seems to be a bit less useful, as it is basically the `common_range`
> ```
> template <class R>
> concept common_range = same_as<range_iterator_t<R>, range_sentinel_t<R>>
> ```
> 
> any suggestions?
> this concept seems to be a bit less useful, as it is basically the `common_range`
I think the main utility of this concept would be its descriptive name, so even as an alias for `common_range` it could have some value (it's local to this file so it's fine for this concept to be not very useful in general). But I don't want to block this patch on this -- feel free to keep things as they are.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/iter.move.pass.cpp:45
+
+    auto it2 = std::ranges::next(it, 4);
+
----------------
huixie90 wrote:
> var-const wrote:
> > huixie90 wrote:
> > > var-const wrote:
> > > > Question: why is checking `iter_move` with another iterator necessary?
> > > If `join_view::iterator` completely forgot to implement `iter_move`, the default implementation of `std::ranges::iter_move` would actually call `std::move(*it)`. The original test point would pass anyway (`std::move(int&) == int&&`)
> > > 
> > > Here I am adding this additional test with a custom underlying iterator where `iter_move` isn't simply `std::move(*it)`, and also it has got an int to track if its own `iter_move` customisation is called
> > Sorry, what I meant is, why are both `iter_move_called_times1` and `iter_move_called_times2` necessary? What case does `iter_move_called_times2` cover that isn't covered by `iter_move_called_times1`?
> since I am joining a range of 2 inner ranges, this is just to make sure that the `iter_move` is called on the iterator of the correct inner range after each step. If you think this is unnecessary I can delete it and just testing joining a range of  1 inner range
It makes sense -- maybe add a short comment to that effect, though.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:367
+  // This is to make default generated iter_move, iter_swap not behave correctly
+  // note: zip_view has exactly same thing below
+  using value_type = std::pair<int, int>;
----------------
huixie90 wrote:
> var-const wrote:
> > Nit: I found this part (about `zip_view`) a little confusing, can you elaborate? If you mean that `zip_view` will have a similar `move_swap_aware_iter` type, then it's better to just omit it -- currently, the test code for different classes is written to be self-contained.
> this is to test `join`ing a range of `zip_view` without actually having `zip_view`. This iterator is emulating proxy-like iterator where `value_type` and `reference` do not simply have the `remove_cvref` relation.  `zip_view`'s iterator is just one of this kind. The test is not testing `zip_view` but joining a range of `zip`-like ranges
How about the following rephrasing?
```
// This is a proxy-like iterator where `reference` is a value type, and `reference` and `value_type`
// are distinct types (similar to `zip_view::iterator`).
```
Though given that, like you say, there are other iterators with similar properties, perhaps the part about it being similar to `zip_view` is unnecessary -- I think I'd drop it and and just keep the explanation about it being a proxy iterator.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123466/new/

https://reviews.llvm.org/D123466