[libcxx-commits] [PATCH] D122806: [libc++] add zip_view and views::zip for C++23
Nikolas Klauser via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Apr 5 14:53:38 PDT 2022
philnik 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)),
----------------
huixie90 wrote:
> 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`
OK, thinking of it that way makes sense. Then I'm OK with `__tuple_zip_transform`.
================
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>());
----------------
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`.
================
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()));
+ }
----------------
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.
================
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())>);
+ }
----------------
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.
================
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<>>);
+ }
----------------
huixie90 wrote:
> 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?
I think I had a bit of a brain-fart here. I can't even remember seeing `auto v = std::views::zip()`. I thought you were just testing `ranges::empty_view` here. Ignore my previous comment.
================
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);
----------------
huixie90 wrote:
> 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.
It's not only about being concise. I can also instantly see what expression is tested. With the current style I first have to look for the declaration of the variable to know what you are testing.
================
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:
> > 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.
================
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};
----------------
huixie90 wrote:
> 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
I meant:
> You should also:
> - check that `begin()` is only available if `!(simple-view<Views> && ...)`
> - check that `begin() const` is only available if `range<const Views> && ...`
> - add runtime tests for `begin() const`
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:32
+
+constexpr bool test() {
+ {
----------------
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<`.
================
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};
----------------
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.
================
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);
----------------
huixie90 wrote:
> 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`
I meant that you should check that `HasIterCategory` itself is written properly and doesn't just always return `false`.
================
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)>);
----------------
huixie90 wrote:
> 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);
> ```
Right... - ignore me.
================
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)>);
+
----------------
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.
================
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};
----------------
huixie90 wrote:
> 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
Yes, but you only check it for one of the overloads. You should also check that `SentinelHasMinus` can return true, and check that it has `operator-` for const/non-const compatible iterator/sentinel pairs, but not for incompatible ones. I think I already mentioned that somewhere, but maybe this thought never left my head.
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