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

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/__ranges/zip_view.h:94
+_LIBCPP_HIDE_FROM_ABI constexpr auto __tuple_zip_transform(_Fun&& __f, _Tuple1&& __tuple1, _Tuple2&& __tuple2) {
+  return __tuple_zip_transform(__f, std::forward<_Tuple1>(__tuple1), std::forward<_Tuple2>(__tuple2),
+                               make_index_sequence<tuple_size<remove_cvref_t<_Tuple1>>::value>());
----------------
philnik wrote:
> huixie90 wrote:
> > philnik wrote:
> > > Same below.
> > it isn't in `std` namespace but `std::ranges` so I changed it to `std::ranges::__tuple_zip_transform`.  This is the case for all the rest of the ADL calls I had below
> > 
> > One puzzle I had is that in my experience, for local helpers in the header, I would put them into a detail namespace, so here I could do `zip_detail::tuple_zip_transform`. What is the reason why a detail namespace isn't used here?
> Putting `ranges::` before the calls is enough. You can use a detail namespace, but you would still have to make it `__zip_detail::__tuple_zip_transform`, so I don't see any improvement over putting it directly inside `ranges`.
i will uses `ranges::`. The reason I like `__zip_detail` namespace is that it is very clear what is implementation details and what is public API. here I guess you would use `__` to indicate implementation details. But that still pollutes the namespace and make my clangd give lots of suggestions with double underscored stuff.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/begin.pass.cpp:42
+    std::ranges::zip_view v(SimpleCommon{buffer}, SimpleCommon{buffer});
+    ASSERT_SAME_TYPE(decltype(v.begin()), decltype(std::as_const(v).begin()));
+  }
----------------
philnik wrote:
> huixie90 wrote:
> > philnik wrote:
> > > Could you use `std::is_same_v` here for consistency? This and the diff above would remove the need for `#include "test_macros.h"`.
> > I am happy to change. but in a previous comment, Marked suggested to use `ASSERT_SAME_TYPE` so I changed some usage of `std::same_as` into `ASSERT_SAME_TYPE`
> 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.
fair enough, i will remove all usage of the macro


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/begin.pass.cpp:46
+    std::ranges::zip_view v(SimpleCommon{buffer}, NonSimpleNonCommon{buffer});
+    static_assert(!std::same_as<decltype(v.begin()), decltype(std::as_const(v).begin())>);
+  }
----------------
philnik wrote:
> huixie90 wrote:
> > philnik wrote:
> > > I think this should just be a `std::is_same_v`. Reads a bit better.
> > what is the reason to favour `std::is_same_v` over `std::same_as`? It reads the same to me
> > 
> > ```
> > static_assert(!std::same_as<decltype(v.begin()), decltype(std::as_const(v).begin())>);
> > ```
> > 
> > vs
> > 
> > ```
> > static_assert(!std::is_same_v<decltype(v.begin()), decltype(std::as_const(v).begin())>);
> > ```
> This is more of a feeling than anything else, so I don't think I can say anything of substance here.
fair enough. I am ok with either way. To make you feel better, I changed all the `same_as` to `is_same_v`


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


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:32
+
+constexpr bool test() {
+  {
----------------
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


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/iter_swap.pass.cpp:21
+
+constexpr bool test() {
+  std::array a1{1, 2, 3, 4};
----------------
philnik wrote:
> huixie90 wrote:
> > philnik wrote:
> > > Same here, you should check that `iter_swap` is passed through to the iteratos.
> > I thought I have checked.  line 30 to line 33 are exactly checking the underlying `iter_swap` is called so that the underlying ranges elements are swapped properly
> I mean that the user-provided `iter_swap` is called. With the current test the implementation could just call `std::swap` and it would behave exactly the same.
sure I would create a iterator with  customised `iter_swap`.

BTW, `std::swap` won't work with the current status of libcxx. This is because
```
using std::swap;
swap(*iter1, *iter2);
```
would not compile, as there is no `swap` overload that can take two prvalue of `std::tuple<int&, int&>`

If you use the triple-move implementation for `std::swap`, it won't work either.

Once we implemented the other section about tuple in this paper, it might make the `swap` version work


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/ctor.other.pass.cpp:57
+
+constexpr bool test() {
+  int buffer1[4] = {1, 2, 3, 4};
----------------
philnik wrote:
> You should check that the constructor is explicit.
The constructor is not explicit. see line63 I am testing the implicit conversions


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/eq.pass.cpp:59
+    static_assert(!std::ranges::common_range<decltype(v)>);
+    LIBCPP_STATIC_ASSERT(std::ranges::__simple_view<decltype(v)>);
+
----------------
philnik wrote:
> huixie90 wrote:
> > philnik wrote:
> > > You shouldn't use implementation details in the tests.
> > I don't think `__simple_view` is an implementation detail per se. The standard has the wording for the exposition-only concept `simple-view`, and is used in lots of places. Here I am testing `zip(NonSimpleNonCommon, SimpleNonCommon)` ends up with a NonSimpleNonCommon view 
> `__simple_view` is an implementation detail of libc++. These tests should also work for other standard libraries which may implement `__simple_view` with a different name or don't implement it at all.
sure. the test won't fail with other implementations because I am using `LIBCPP_STATIC_ASSERT`. 

`LIBCPP_STATIC_ASSERT` is not the verification. It is just to double check I have a sentinel. The tests that testing `simple-view`-ness is in a different file. 

the actual tests here are the lines below which exercise the `==` 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122806