[libcxx-commits] [PATCH] D122806: [libc++] add zip_view and views::zip for C++23
Hui via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Apr 5 14:00:10 PDT 2022
huixie90 marked 20 inline comments as done.
huixie90 added inline comments.
================
Comment at: libcxx/include/__ranges/zip_view.h:87
+ typename tuple_element<_Indices, remove_cvref_t<_Tuple2>>::type>...>
+__tuple_zip_transform(_Fun&& __f, _Tuple1&& __tuple1, _Tuple2&& __tuple2, index_sequence<_Indices...>) {
+ return {std::invoke(__f, std::get<_Indices>(std::forward<_Tuple1>(__tuple1)),
----------------
philnik wrote:
> I think it would be nice to have a more meaningful name for these functions. They are way to easy to mix up with `__tuple_transform` and the like. Maybe something like `__binary_tuple_transform`? I'm not sure. Maybe someone else has got a good idea.
It is really a `zip_transform`, but for `tuples` instead of `ranges`.
For ranges, `zip_transform(f, r1, r2)` will zip r1 and r2 and apply `f` to each of the zipped element, effectively
```
[ f(r1[0], r2[0]), f(r1[1], r2[1]), ...]
```
Here, we have the same logic, but for tuples instead of ranges
`tuple_zip_transform(f, t1, t2)` is effectively
```
tuple( f(get<0>(t1), get<0>(t2), f(get<1>(t1), get<1>(t2)), ...)
```
so it is `zip_transform` for `tuple`s ( and in theory I could implement it to take variadic number of `tuple`s but it is not needed. Therefore, `tuple_zip_transform` is a reasonable name to me. or could call it `zip_transform_for_tuple`
================
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:
> 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?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/begin.pass.cpp:31-34
+ auto it = v.begin();
+ assert(*it == std::make_tuple(1, 0, 2.0));
+ assert(&(std::get<0>(*it)) == &buffer[0]);
+ ASSERT_SAME_TYPE(decltype(*it), std::tuple<int&, int, double&>);
----------------
philnik wrote:
> Just a suggestion
The reason I didn't use `tuple`'s CTAD is that it crashes gcc-11.2
================
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:
> 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`
================
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:
> 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())>);
```
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/cpo.pass.cpp:27-33
+ {
+ auto v = std::views::zip();
+ assert(std::ranges::empty(v));
+ // a bit weird. non 0-arity zipped range's reference is a prvalue tuple
+ ASSERT_SAME_TYPE(std::ranges::range_reference_t<decltype(v)>, std::tuple<>&);
+ ASSERT_SAME_TYPE(decltype(v), std::ranges::empty_view<std::tuple<>>);
+ }
----------------
philnik wrote:
> It doesn't look like you are testing zip here.
the spec says `views::zip()` should have the same effect of `views::empty<tuple<>>`.
I think I am testing that. could you elaborate?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/cpo.pass.cpp:36
+ int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
+ auto v = std::views::zip(SizedRandomAccessView{buffer});
+ assert(std::ranges::size(v) == 8);
----------------
philnik wrote:
> And then remove the `ASSERT_SAME_TYPE`?
I am happy to change but I am not convinced.
what you suggested is indeed more concise, but I still prefer separating creating the variable, and the expression that does the verification for the test in two statements.
================
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:
> 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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/end.pass.cpp:21
+
+constexpr bool test() {
+ int buffer1[5] = {1, 2, 3, 4, 5};
----------------
philnik wrote:
> You seem to be missing the same tests here as for `begin()`.
what exactly tests you are referring to? `begin` is much much more simpler than `end`, because `end` needs to deal with underlying ranges having different sizes
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:32
+
+constexpr bool test() {
+ {
----------------
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
================
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:
> 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
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/member_types.compile.pass.cpp:81
+ ASSERT_SAME_TYPE(Iter::iterator_concept, std::input_iterator_tag);
+ static_assert(!HasIterCategory<Iter>);
+ ASSERT_SAME_TYPE(Iter::difference_type, std::ptrdiff_t);
----------------
philnik wrote:
> Could you add a check to make sure `HasIterCategory` is true when it's supposed to?
All other test points are already checking the `iterator_category` being `input_iterator_tag`
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/ctor.other.pass.cpp:63
+ auto sent1 = v.end();
+ std::ranges::sentinel_t<const decltype(v)> sent2 = sent1;
+ static_assert(!std::same_as<decltype(sent1), decltype(sent2)>);
----------------
philnik wrote:
> Why not `auto sent2 = std::as_const(sent1)`?
no this is incorrect.
```
std::ranges::sentinel_t<const decltype(v)> sent2 = sent1;
```
is basically doing
```
zip_view::__sentinel<false> sent1 = .....;
zip_view::__sentinel<true> sent2 = sent1;
```
whereas
```
auto sent2 = std::as_const(sent1)
```
is basically doing
```
zip_view::__sentinel<false> sent1 = .....;
const zip_view::__sentinel<false> sent2 = as_const(sent1);
```
================
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:
> 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
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/minus.pass.cpp:36
+
+constexpr bool test() {
+ int buffer1[5] = {1, 2, 3, 4, 5};
----------------
philnik wrote:
> You should have a test to make sure `operator-` is only available if it's a sized_sentinel.
I think I already have it. the last test point is zipping a range with non_sized sentinel with a range with sized sentinel. so the zipped range does't have sized sentinel. I will add a comment to explicitly express that
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122806/new/
https://reviews.llvm.org/D122806
More information about the libcxx-commits
mailing list