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

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
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]>);
+
----------------
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.


================
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));
 
----------------
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?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/iter.move.pass.cpp:45
+
+    auto it2 = std::ranges::next(it, 4);
+
----------------
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


================
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>;
----------------
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


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:143
+template <class T>
+struct Buffer{
+   T* buffer_;
----------------
var-const wrote:
> huixie90 wrote:
> > var-const wrote:
> > > From the name, I would expect a buffer to own the memory, whereas this is non-owning view. Can you use `std::span`?
> > I will rename it. I'd like to have access to the underlying pointer and I don't think `std::span` has that property
> > I don't think `std::span` has that property
> Wouldn't `data()` work?
that `Span`/`Buffer` class is deleted now. Now I only have the `BufferView` template where its template parameters are iterators and sentinels.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123466