[libcxx-commits] [PATCH] D122806: [libc++] add zip_view and views::zip for C++23

Thu Apr 14 06:34:56 PDT 2022

philnik added a comment.

This looks really good so far. It's mostly nits from my side.

================
Comment at: libcxx/docs/Status/ZipProjects.csv:11-13
+| `zip_view::sentinel`", Hui Xie, |Complete|
+| `[range.zip.iterator] <https://wg21.link/range.zip.transform>`_, "zip_view::iterator", None, Hui Xie, |Complete|
+| `[range.zip.sentinel] <https://wg21.link/range.zip.sentinel>`_, "zip_view::sentinel", None, Hui Xie, |Complete|
----------------
Could you add the link to this patch?

================
Comment at: libcxx/include/__ranges/zip_view.h:340
+    } else {
+      const auto __it_equals = __tuple_zip_transform(std::equal_to<>(), __x.__current_, __y.__current_);
+      return std::apply([](auto... __bs) { return (__bs || ...); }, __it_equals);
----------------

================
Comment at: libcxx/include/__ranges/zip_view.h:434
+  {
+    __tuple_zip_for_each(ranges::iter_swap, __l.__current_, __r.__current_);
+  }
----------------

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/borrowing.compile.pass.cpp:35-41
+void testBorrowedRange() {
+  static_assert(std::ranges::borrowed_range<std::ranges::zip_view<Borrowed>>);
+  static_assert(std::ranges::borrowed_range<std::ranges::zip_view<Borrowed, Borrowed>>);
+  static_assert(!std::ranges::borrowed_range<std::ranges::zip_view<Borrowed, NonBorrowed>>);
+  static_assert(!std::ranges::borrowed_range<std::ranges::zip_view<NonBorrowed>>);
+  static_assert(!std::ranges::borrowed_range<std::ranges::zip_view<NonBorrowed, NonBorrowed>>);
+}
----------------
You don't have to put the `static_assert`s inside a function.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctad.compile.pass.cpp:33-34
+void testCTAD() {
+  ASSERT_SAME_TYPE(decltype(std::ranges::zip_view(Container{})),
+                   std::ranges::zip_view<std::ranges::owning_view<Container>>);
+
----------------
huixie90 wrote:
> philnik wrote:
> > huixie90 wrote:
> > > philnik wrote:
> > > > Through what magic do they become `owning_view`s? You also don't have to have these `assert`s inside a function.
> > > P2415R2 is DRed back to C++20
> > > 
> > > I'd prefer to having the local variable `c` inside a function rather than a global variable. 
> > You want `c` to be an lvalue, right? so why not make it `std::declval<Container&>()`? That would remove the need for the variable all-together.
> I used to do `std::declval` a lot in this kind of tests, but in a previous review Arthur pointed out that the test is much less readable than just create a normal variable like normal user would do. So I think I agree with him in this case.
OK, I don't have a strong opinion here. You can leave it as-is if you think it's easier to read.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.views.pass.cpp:20
+
+constexpr bool test() {
+
----------------
philnik wrote:
> You should also check here that
> - the arguments are always moved
> - the arguments are moved exactly once
> - all the different range types (`input_range`, `forward_range`, `output_range` etc.); See the any of the ranges algorithm tests for an example of that.
> - the constructor is `explicit`
It looks like you aren't checking the that `zip` accepts all the ranges types.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/end.pass.cpp:20-66
+// ID | simple | common | bidi | random | sized | #views |     v.end()    | as_const(v)
+//    |        |        |      | access |       |        |                |   .end()
+// ---|--------|--------|------|--------|-------|--------|----------------|---------------
+// 1  |   Y    |   Y    |  Y   |    Y   |   Y   |   1    | iterator<true> | iterator<true>
+// 2  |   Y    |   Y    |  Y   |    Y   |   Y   |   >1   | iterator<true> | iterator<true>
+// 3  |   Y    |   N    |  Y   |    Y   |   N   |   1    | sentinel<true> | sentinel<true>
+// 4  |   Y    |   N    |  Y   |    Y   |   N   |   >1   | sentinel<true> | sentinel<true>
----------------
I really like this! Thanks!

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:32
+
+constexpr bool test() {
+  {
----------------
huixie90 wrote:
> philnik wrote:
> > huixie90 wrote:
> > > philnik wrote:
> > > > You should check here that all the operators are passed through properly. So you should make an iterator that deletes all comparison operators other than `operator<` and then count the invocations of that operator.
> > > I am not sure if I understand. `random_access_iterator` requires all these operators. If I delete all others with only `operator<` , then it won't model `random_access_iterator` and the `zip`'s iterator won't have `operator<` at all
> > Right, then I think you should only `assert(false)` in the other comparators. Although maybe you have to count the number of calls, because there are probably semantic requirements that make using `assert(false)` IFNDR. AFAICT it's mandatory that the implementation uses `operator<`.
> for the sake of simplicity. I declared other operators but not implemented. it is declared so that the random access concept check would pass.
> 
> If zip_view is going to use those operators, it would fail the compile time test because these unimplemented operators are not constexpr.  Also, it will fail to link if they are used because there are no implementations.
> 
> It works for clang trunk and gcc 11.2.  i will check if this approach can pass the CI
That's also OK.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/iter_move.pass.cpp:28
+
+constexpr bool test() {
+  {
----------------
philnik wrote:
> You should check here that the `iter_move` of the iterator is called properly.
I think you aren't checking that a custom `iter_move` is called properly. This should be quite similar to the `iter_swap` test.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/range.concept.compile.pass.cpp:320
+static_assert(!zippable<SimpleCommon, OutputView>);
\ No newline at end of file

----------------
Could you add a newline?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/types.h:18-20
+#if TEST_STD_VER < 20
+#  error "range.zip/types.h" can only be included in builds supporting C++20
+#endif // TEST_STD_VER < 20
----------------
This header should only ever be included by the `zip` tests, right?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122806