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

Tue Apr 19 15:48:01 PDT 2022

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Thanks for addressing the feedback (and sorry about the slow review)! Almost LGTM with just a few nits/questions.

================
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:
> Question: why not use `same_as` like above?
Seems not done, unless I'm missing something.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp:85
+    assert(&(*v.begin()) == buffer1);
+    static_assert(CanBePiped<decltype((inners)), decltype((std::views::join))>);
+  }
----------------
huixie90 wrote:
> huixie90 wrote:
> > var-const wrote:
> > > var-const wrote:
> > > > Question: why are the double parentheses necessary? (here and below)
> > > Does this check provide any additional coverage? It seems like it does the same as `inners | views::join`, except it doesn't exercise the runtime part.
> > `decltype(inner)` is `Inner[3]`, but `decltype((inner))` is `Inner (&) [3]`, which is an lvalue reference.  In expression `inners | views::join`, `inners` is lvalue reference
> This doesn't add additional coverage. However, this was suggested by @philnik, to check that the local helper concept `CanBePiped` is able to return `true`. (as supposed `concept CanBePiped = false` that renders the negative test useless
Thanks for explaining, makes sense.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/ctor.default.pass.cpp:32
+
+  // Default constructor should default initialise underlying view
+  {
----------------
Shouldn't it be `value initialise`? Default initialization would leave `i` uninitialized.

================
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:
> > 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.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/arrow.pass.cpp:128
+    // LWG3500 `join_view::iterator::operator->()` is bogus
+    // `operator->` is not defined if inner iterator is not copyable
+    // has-arrow<InnerIter> && !copyable<InnerIter>
----------------
Ultranit: I think `should not be defined` would be a little clearer, but it's a really small thing, so feel free to ignore (below as well).

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/ctor.parent.outer.pass.cpp:40
+    // even if it is not default constructible
+    // This test is checking that this constructor can be invoked with an anner range with non default
+    // constructible iterator
----------------
Nit: `s/anner/inner/`?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/decrement.pass.cpp:77
+  {
+#if defined(__GNUG__) && !defined(__clang__)
+    if (!std::is_constant_evaluated()) {
----------------
huixie90 wrote:
> var-const wrote:
> > Why is this necessary?
> for some reason, gcc fails the compile time test (fine at runtime). I think it is a gcc bug ( i need to find a minimum reproducible example)
Please add a comment explaining the issue (if you already have filed a GCC bug or of it's easy to do, add a link to the comment, but I wouldn't block this patch on filing the GCC bug). In general, when doing this sort of compiler- or platform-specific workarounds, it's always helpful to leave a comment explaining the issue.

================
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:
> > 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`?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/member_types.compile.pass.cpp:59

-    ASSERT_SAME_TYPE(Iter::iterator_concept, std::bidirectional_iterator_tag);
-    ASSERT_SAME_TYPE(Iter::iterator_category, std::bidirectional_iterator_tag);
-    ASSERT_SAME_TYPE(Iter::difference_type, std::ptrdiff_t);
-    ASSERT_SAME_TYPE(Iter::value_type, int);
+    static_assert(std::is_same_v<Iter::iterator_concept, std::bidirectional_iterator_tag>);
+    static_assert(std::is_same_v<Iter::iterator_category, std::bidirectional_iterator_tag>);
----------------
huixie90 wrote:
> var-const wrote:
> > What is the reason for this change?
> On another review, @philnik was skeptical about the macro `ASSERT_SAME_TYPE`. Here is his comment
> 
> > I'm not a huge fan of ASSERT_SAME_TYPE in general. It seems to me like a macro that is there for a very small amount of convenience and makes the code much harder to read IMO. At least I haven't seen a case where it makes life a lot easier.
> 
> 
> I was following his suggestion to not to use it. happy to revert. I am ok with either way. what is your opinion?
Thanks for explaining. Re. `ASSERT_SAME_TYPE` -- personally, I actually find it a little easier to read with less boilerplate, but it's definitely not worth it to go and change it back now. I don't think there ever was a general agreement to stop using `ASSERT_SAME_TYPE`, so I think it comes down to personal preference (@philnik, please correct me if I'm wrong on this).

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/member_types.compile.pass.cpp:113
+    using Iter = std::ranges::iterator_t<std::ranges::join_view<Outer>>;
+    static_assert(std::is_same_v<Iter::difference_type, std::common_type_t<std::intptr_t, std::ptrdiff_t>>);
   }
----------------
huixie90 wrote:
> var-const wrote:
> > Question: can this check be more specific?
> could you please elaborate? This test point is checking `difference_type` is defined correctly, which should be the common_type of the inner and outer `difference_type`
Never mind, on a second thought I think the current state is reasonable (I was going to suggest using a more specific type but it would be non-portable, and trying to work around that is not worth the trouble).

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

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:143
+template <class T>
+struct Buffer{
+   T* buffer_;
----------------
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?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:286
+template <class It>
+struct copy_iterator
+{
----------------
huixie90 wrote:
> var-const wrote:
> > What is the purpose of this class?
> It is an `iterator` where its `reference` is a prvalue. It is used in some of the tests where the inner range produces prvalue `reference`
Thanks for explaining. Optionally, consider renaming it to `copying_iterator`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123466