[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 12:29:41 PDT 2022
philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.
The implementation looks mostly good. There are still a few ADL-calls and some non-_Uglified names. The tests need some work. Mostly checking constraints.
================
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)),
----------------
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.
================
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>());
----------------
Same below.
================
Comment at: libcxx/include/__ranges/zip_view.h:109
+ return __tuple_zip_for_each(__f, std::forward<_Tuple1>(__tuple1), std::forward<_Tuple2>(__tuple2),
+ make_index_sequence<tuple_size<remove_cvref_t<_Tuple1>>::value>());
+}
----------------
================
Comment at: libcxx/include/__ranges/zip_view.h:115
+template <class _Tp>
+_LIBCPP_HIDE_FROM_ABI constexpr _Tp __abs(_Tp t) {
+ return t < 0 ? -t : t;
----------------
================
Comment at: libcxx/include/__ranges/zip_view.h:142
+ {
+ return __iterator<false>(__tuple_transform(ranges::begin, __views_));
+ }
----------------
Same below.
================
Comment at: libcxx/include/__ranges/zip_view.h:411
+ {
+ const auto __diffs = __tuple_zip_transform(std::minus<>(), __x.__current_, __y.__current_);
+ return std::apply(
----------------
You can, you don't have to.
================
Comment at: libcxx/include/__ranges/zip_view.h:414
+ [](auto... __ds) {
+ return ranges::min({difference_type(__ds)...}, [](auto __a, auto __b) { return __abs(__a) < __abs(__b); });
+ },
----------------
================
Comment at: libcxx/include/__ranges/zip_view.h:518
+inline namespace __cpo {
+inline constexpr auto zip = __zip::__fn{};
+} // namespace __cpo
----------------
================
Comment at: libcxx/include/__ranges/zip_view.h:520
+} // namespace __cpo
+
+} // namespace views
----------------
Can you remove these empty lines between the closing namespaces?
================
Comment at: libcxx/test/libcxx/diagnostics/detail.headers/ranges/zip_view.module.verify.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Friendly reminder: please rebase and remove this file.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/begin.pass.cpp:27
+
+constexpr bool test() {
+ int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
----------------
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/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&>);
----------------
Just a suggestion
================
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()));
+ }
----------------
Could you use `std::is_same_v` here for consistency? This and the diff above would remove the need for `#include "test_macros.h"`.
================
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())>);
+ }
----------------
I think this should just be a `std::is_same_v`. Reads a bit better.
================
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<>>);
+ }
----------------
It doesn't look like you are testing zip here.
================
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);
----------------
And then remove the `ASSERT_SAME_TYPE`?
================
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>>);
+
----------------
Through what magic do they become `owning_view`s? You also don't have to have these `assert`s inside a function.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.default.pass.cpp:44-46
+ assert(*it++ == Pair(buff[0], buff[0]));
+ assert(*it++ == Pair(buff[1], buff[1]));
+ assert(*it == Pair(buff[2], buff[2]));
----------------
This seems to go a bit into testing `operator++(int)`, but I think it's OK in this case.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.default.pass.cpp:47
+ assert(*it == Pair(buff[2], buff[2]));
+ static_assert(std::is_default_constructible_v<decltype(v)>);
+ }
----------------
This `static_assert` seems unnecessary. You already check this with line 40.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.default.pass.cpp:49-57
+ {
+ std::ranges::zip_view<DefaultConstructibleView, DefaultConstructibleView> v{};
+ assert(v.size() == 3);
+ }
+ {
+ using View = std::ranges::zip_view<DefaultConstructibleView, DefaultConstructibleView>;
+ View v = View();
----------------
These tests seem to do the exact same thing as the first one. If you want to check that the default constructor isn't explicit, just do it in the first test case and add a comment, so no one removes this check accidentally.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.default.pass.cpp:58-60
+ static_assert(!std::is_default_constructible_v< std::ranges::zip_view<DefaultConstructibleView, NoDefaultView>>);
+
+ static_assert(!std::is_default_constructible_v<std::ranges::zip_view<NoDefaultView>>);
----------------
Could you put the `static_cassert`s above the runtime test?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.views.pass.cpp:20
+
+constexpr bool test() {
+
----------------
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`
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.views.pass.cpp:31
+
+ {
+ std::ranges::zip_view v{SizedRandomAccessView{buffer}, std::views::iota(0), std::ranges::single_view(2.)};
----------------
philnik wrote:
> Again, this seems to be a duplicate of the first test.
Could you add `// comments everywhere to say what you are testing for`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.views.pass.cpp:31-37
+ {
+ std::ranges::zip_view v{SizedRandomAccessView{buffer}, std::views::iota(0), std::ranges::single_view(2.)};
+ auto [i, j, k] = *v.begin();
+ assert(i == 1);
+ assert(j == 0);
+ assert(k == 2.0);
+ }
----------------
Again, this seems to be a duplicate of the first test.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/end.pass.cpp:17
+
+#include "test_macros.h"
+#include "test_range.h"
----------------
It looks like you aren't using this include anywhere.
================
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};
----------------
You seem to be missing the same tests here as for `begin()`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/end.pass.cpp:26
+ {
+ // underlying simple non-common non-sized (zip non-common)
+ std::ranges::zip_view v(SimpleNonCommon{buffer3}, SimpleCommon(buffer1));
----------------
Could you maybe try to make the comments look a bit like a matrix? That would make it much easier to check that you have checked everything.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/arithmetic.pass.cpp:26
+
+constexpr bool test() {
+
----------------
Here you are missing tests for:
- checking that the operations are only available for all-random-access iterators
- `operator-(iter, iter)` is only available with sized sentinels
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:32
+
+constexpr bool test() {
+ {
----------------
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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/ctor.default.pass.cpp:17
+
+#include "test_macros.h"
+#include "../types.h"
----------------
here you are also not using `test_macros.h` AFAICT. Could you check the other tests for unused includes?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/ctor.default.pass.cpp:45
+ using It = zip_iter<Default>;
+ [[maybe_unused]] It it;
+ static_assert(std::default_initializable<It>);
----------------
You should check here that the inner iterators are default initialized. I think I already left a comment about that somewhere.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/deref.pass.cpp:22
+
+constexpr bool test() {
+ std::array a{1, 2, 3, 4};
----------------
You should check here that `operator*` is const.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/increment.pass.cpp:54
+ ASSERT_SAME_TYPE(decltype(it++), Iter);
+ it++;
+ assert(&(std::get<0>(*it)) == &(a[2]));
----------------
You aren't checking the return value of `it++`;
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/increment.pass.cpp:76
+ assert(&(std::get<0>(*it)) == &(buffer[2]));
+ }
+ {
----------------
You aren't checking that `operator++(int)` for a `forward_range` is iterator.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/iter_move.pass.cpp:28
+
+constexpr bool test() {
+ {
----------------
You should check here that the `iter_move` of the iterator is called properly.
================
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};
----------------
Same here, you should check that `iter_swap` is passed through to the iteratos.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/iter_swap.pass.cpp:26
+ auto iter1 = v.begin();
+ auto iter2 = std::next(v.begin());
+
----------------
Why not just `++v.begin()`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/member_types.compile.pass.cpp:37
+
+void test() {
+ {
----------------
Here you should add tests for:
- `difference_type`
- `value_type`s with elements other than `int`
- `const` iterators
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/member_types.compile.pass.cpp:41
+ std::ranges::zip_view v(buffer, buffer);
+ using Iter = std::ranges::iterator_t<decltype(v)>;
+
----------------
I think this should just be `decltype(v.begin())`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/member_types.compile.pass.cpp:46
+ ASSERT_SAME_TYPE(Iter::difference_type, std::ptrdiff_t);
+ ASSERT_SAME_TYPE(Iter::value_type, std::pair<int, int>);
+ }
----------------
Could you add a test where there are elements other than `int`?
================
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);
----------------
Could you add a check to make sure `HasIterCategory` is true when it's supposed to?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/ctor.default.pass.cpp:24-26
+ [[maybe_unused]] Sentinel st1;
+ [[maybe_unused]] Sentinel st2 = {};
+
----------------
Again, you should check that the sentinels are default constructed.
================
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};
----------------
You should check that the constructor is explicit.
================
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)>);
----------------
Why not `auto sent2 = std::as_const(sent1)`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/eq.pass.cpp:52
+
+constexpr bool test() {
+ int buffer1[4] = {1, 2, 3, 4};
----------------
You should check that `operator==` isn't available if const and non-const sentinel/iterator pairs aren't compatible.
================
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)>);
+
----------------
You shouldn't use implementation details in the tests.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/minus.pass.cpp:34
+template <class T>
+concept SentinelMinusable = requires(T t) { t.end() - t.begin(); };
+
----------------
Sounds a lot better IMO.
================
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};
----------------
You should have a test to make sure `operator-` is only available if it's a sized_sentinel.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sized.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I guess this file should be named `size.pass.cpp`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sized.pass.cpp:40
+
+constexpr bool test() {
+ {
----------------
Again, you are missing tests for const-ness of `size()` and availability.
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