[libcxx-commits] [PATCH] D122806: [libc++] add zip_view and views::zip for C++23
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Apr 21 16:54:23 PDT 2022
var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.
Sorry about the slow review. Thanks a lot for working on this huge patch!
One comment I have that applies throughout most tests -- can you please add a brief comment to each test case to explain what exactly it tests? Even a short description makes things easier to skim through. Additionally, could you please separate the test cases by blank lines? It's a minor nit, but I think it makes it a little easier to see where one test case ends and another one starts.
================
Comment at: libcxx/include/__ranges/zip_view.h:84
+template <class _Fun, class _Tuple1, class _Tuple2, size_t... _Indices>
+_LIBCPP_HIDE_FROM_ABI constexpr __tuple_or_pair<
+ invoke_result_t<_Fun&, typename tuple_element<_Indices, remove_cvref_t<_Tuple1>>::type,
----------------
Is specifying the complicated return type necessary here (i.e., could this function get away with just `auto`)?
================
Comment at: libcxx/include/__ranges/zip_view.h:240
+template <bool _Const>
+class zip_view<_Views...>::__iterator : public __zip_view_iterator_category_base<_Const, _Views...> {
+
----------------
Is the following section implemented and tested?
```
If the invocation of any non-const member function of `iterator` exits via an exception, the iterator acquires a singular value.
```
(http://eel.is/c++draft/range.zip#iterator-3)
================
Comment at: libcxx/include/__ranges/zip_view.h:286
+ requires __zip_all_forward<_Const, _Views...>
+ {
+ auto __tmp = *this;
----------------
Nit: a few of these functions should be formatted so that the opening brace is on the same line as the function name.
================
Comment at: libcxx/include/__ranges/zip_view.h:341
+ const auto __it_equals = ranges::__tuple_zip_transform(std::equal_to<>(), __x.__current_, __y.__current_);
+ return std::apply([](auto... __bs) { return (__bs || ...); }, __it_equals);
+ }
----------------
Nit: can `__bs` be a little more descriptive?
================
Comment at: libcxx/include/__ranges/zip_view.h:394
+ {
+ return __i + __n;
+ }
----------------
Why is this implementation different from the overload above (and from the Standard)?
================
Comment at: libcxx/include/__ranges/zip_view.h:429
+ friend constexpr void iter_swap(const __iterator& __l, const __iterator& __r) noexcept(
+ (noexcept(ranges::iter_swap(declval<const iterator_t<__maybe_const<_Const, _Views>>&>(),
+ declval<const iterator_t<__maybe_const<_Const, _Views>>&>())) &&
----------------
Hmm, is this equivalent to the Standard's wording?
```
The exception specification is equivalent to the logical AND of the following expressions:
noexcept(ranges::iter_swap(std::get<i>(l.current_), std::get<i>(r.current_)))
for every integer `0 ≤ i < sizeof...(Views)`.
```
================
Comment at: libcxx/include/__ranges/zip_view.h:471
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const __iterator<_OtherConst>& __x, const __sentinel& __y) {
+ const auto __it_equals = ranges::__tuple_zip_transform(std::equal_to<>(), __iter_current(__x), __y.__end_);
+ return std::apply([](auto... bs) { return (bs || ...); }, __it_equals);
----------------
Optional: since this pattern is used twice (here and in `__iterator`), consider splitting it into a function (e.g. `__tuple_zip_any_of`).
================
Comment at: libcxx/include/__ranges/zip_view.h:513
+ noexcept(noexcept(zip_view(std::forward<_Ranges>(rs)...))) -> decltype(zip_view(std::forward<_Ranges>(rs)...)) {
+ return zip_view(std::forward<_Ranges>(rs)...);
+ }
----------------
Hmm, looking at the latest draft, it says
```
Given a pack of subexpressions `Es...`, the expression `views::zip(Es...)` is expression-equivalent to
<...>
(2.2) otherwise, `zip_view<views::all_t<decltype((Es))>...>(Es...)`.
```
This does not appear equivalent (missing `views::all_t`).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/begin.pass.cpp:13
+// constexpr auto begin() requires (!(simple-view<Views> && ...));
+// constexpr auto begin() const requires (range<const Views> && ...);
+
----------------
Is a case where neither of the overloads is available? (i.e., is it possible for a type to not satisfy `range<const Views> && ...` while still satisfying the class constraints?) If yes, can you add a compile-time test to check that case?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/borrowing.compile.pass.cpp:35
+
+// test borrwed_range
+static_assert(std::ranges::borrowed_range<std::ranges::zip_view<Borrowed>>);
----------------
Nit: `s/borrwed/borrowed/`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/cpo.pass.cpp:12
+
+// std::views::zip
+
----------------
This file should also check piping `zip`. Can you take a look at `test/std/ranges/range.adaptors/range.filter/adaptor.pass.cpp` and implement the relevant checks from that test here?
Also, I'd rename it into `adaptor.pass.cpp` for consistency with other views.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.default.pass.cpp:32
+
+struct NoDefaultView : std::ranges::view_base {
+ NoDefaultView() = delete;
----------------
Consider renaming to `NoDefaultCtrView`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.default.pass.cpp:41
+ std::is_default_constructible_v<std::ranges::zip_view<DefaultConstructibleView, DefaultConstructibleView>>);
+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, NoDefaultView>>);
----------------
Hmm, I don't see a constraint on either the default constructor or the class itself that requires `Views` to be default-constructible. I'm probably missing it -- can you please check?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.views.pass.cpp:27
+static_assert(std::constructible_from<std::ranges::zip_view<SimpleCommon>, SimpleCommon>);
+static_assert(!implicitly_constructible_from<std::ranges::zip_view<SimpleCommon>, SimpleCommon>);
+
----------------
Wouldn't `std::convertible_to` work?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.views.pass.cpp:39
+ constexpr const int* begin() const { return &moves; }
+ constexpr const int* end() const { return &moves + 1; }
+};
----------------
Seems like this could be UB? Consider making `moves` a one-element array.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.views.pass.cpp:60
+ auto [numMoves1, numMoves2] = *v.begin();
+ assert(numMoves1 == 2); // one move from stack to parameter, one move from parameter to member
+ assert(numMoves2 == 1);
----------------
Nit: `s/stack/the local variable/` -- the parameter is also on the stack, after all.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.views.pass.cpp:66
+ {
+ std::ranges::zip_view v{InputCommonView{buffer}, ForwardSizedView{buffer2}};
+ auto [i, j] = *v.begin();
----------------
Optional: these checks are so similar that they could be extracted into a helper function that takes two template arguments for the view types to create. But the current state is also fine since there are only 3 similar cases (extraction would allow you to add more cases easily if you'd like).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/end.pass.cpp:77
+ assert(v.begin() + 5 == v.end());
+ static_assert(std::is_same_v<decltype(v.begin()), decltype(std::as_const(v).begin())>);
+ }
----------------
Shouldn't these check `v.end()` rather than `v.begin()`? (applies throughout)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/arithmetic.pass.cpp:12
+
+// x += n;
+// x + n;
----------------
Please specify the constraints (I'd be okay with just writing something like:
```
// All the arithmetic operators have the constraint `requires all-random-access<Const, Views...>;`,
// except `operator-` which has the constraint `requires (sized_sentinel_for<iterator_t<maybe-const<Const, Views>>,
iterator_t<maybe-const<Const, Views>>> && ...);`
```
(of course, double check the above for correctness if you decide to use it)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/arithmetic.pass.cpp:34
+constexpr bool test() {
+
+ int buffer1[5] = {1, 2, 3, 4, 5};
----------------
Nit: this blank line seems extraneous.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/arithmetic.pass.cpp:63
+ using Iter = decltype(it1);
+ static_assert(std::invocable<std::plus<>, Iter, intptr_t>);
+ static_assert(std::invocable<std::plus<>, intptr_t, Iter>);
----------------
These checks seem unnecessary -- it's already checked at runtime above (applies below as well).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/arithmetic.pass.cpp:97
+ // in this case sentinel is computed by getting each underlying sentinel, so the distance
+ // between begin and end for each underlying iterators can be different
+ std::ranges::zip_view v{ForwardSizedView(buffer1), ForwardSizedView(buffer2)};
----------------
Nit: `s/each/each of the/`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/arithmetic.pass.cpp:111
+ {
+ // One of range is not random access
+ std::ranges::zip_view v(a, b, ForwardSizedView{buffer1});
----------------
Nit: `s/range/the ranges/` (here and below).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:12
+
+// zip_view::<iterator>::operator{<,>,<=,>=,==,!=,<=>}
+
----------------
Please add the constraints to the synopsis.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:21
+template <class T>
+concept canSmallerThan = requires(T&& t1, T&& t2) { t1 < t2; };
+
----------------
Also check for the presence of `operator==`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:23
+
+struct smaller_than_iterator {
+ int* it_ = nullptr;
----------------
Same nit as another file: it's a little inconsistent that this class name is in `snake_case`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:23
+
+struct smaller_than_iterator {
+ int* it_ = nullptr;
----------------
var-const wrote:
> Same nit as another file: it's a little inconsistent that this class name is in `snake_case`.
Nit: `s/smaller/less/` (throughout the file). I think `less` is more commonly used in the arithmetic context.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:33
+ constexpr int& operator*() const { return *it_; }
+ constexpr int& operator[](difference_type n) const { return it_[n]; }
+ constexpr smaller_than_iterator& operator++() {
----------------
Question: is defining `operator[]` necessary?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:69
+ constexpr friend bool operator==(smaller_than_iterator const&, smaller_than_iterator const&) = default;
+ friend bool operator!=(smaller_than_iterator const&, smaller_than_iterator const&);
+
----------------
Is this declaration necessary? It seems to go against the purpose of the test, which IIUC verifies that having just `operator==` is sufficient.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:74
+ }
+ friend bool operator<=(smaller_than_iterator const&, smaller_than_iterator const&);
+ friend bool operator>(smaller_than_iterator const&, smaller_than_iterator const&);
----------------
Question: do these operators have to be declared?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:99
+ using It = three_way_contiguous_iterator<int*>;
+ using SubRange = std::ranges::subrange<It>;
+ static_assert(std::three_way_comparable<It>);
----------------
Ultranit: `s/SubRange/Subrange/` (the Standard considers it a single word).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:110
+
+ assert(!(iter1 < iter1));
+ assert(iter1 < iter2);
----------------
These checks are verbose and, as far as I can see, same across the test cases -- can you move them to a helper function?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:186
+ using Iter = decltype(it1);
+ static_assert(!canSmallerThan<Iter>);
+ }
----------------
Please also check that `operator==` is unavailable, and ideally all other operators.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/ctor.default.pass.cpp:34
+
+struct Default : std::ranges::view_base {
+ PODIter begin() const;
----------------
Question: what does "default" and "non-default" mean in the context of these classes?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/ctor.default.pass.cpp:47
+
+static_assert(!std::default_initializable<zip_iter<NonDefault>>);
+static_assert(!std::default_initializable<zip_iter<NonDefault, Default>>);
----------------
Similar question to `zip_view`'s default constructor -- where does this constraint come from?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/ctor.default.pass.cpp:54
+constexpr bool test() {
+ using ZipIter = std::ranges::iterator_t<std::ranges::zip_view<Default>>;
+ {
----------------
Nit: can this be `= zip_iter<Default>` (or just use `zip_iter<Default>` without an alias)?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/ctor.other.pass.cpp:12
+
+// constexpr iterator(iterator<!Const> i);
+
----------------
Missing `requires` clause.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/ctor.other.pass.cpp:33
+ static_assert(!std::constructible_from<decltype(iter1), decltype(iter2)>);
+
+ return true;
----------------
Can you also check the case when the conversion is from non-const to const, but `(!convertible_to<iterator_t<Views>, iterator_t<maybe-const<Const, Views>>> && ...)`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/decrement.pass.cpp:13
+// constexpr iterator& operator--();
+// constexpr iterator operator--(int);
+
----------------
Nit: this synopsis is missing the `requires` clause.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/decrement.pass.cpp:23
+template <class Iter>
+concept canDecrement = requires(Iter it) { --it; };
+
----------------
Optional: probably overkill, but it could also check the post-decrement version (using "or").
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/decrement.pass.cpp:63
+
+ ++it;
+ ++it;
----------------
What's the reason to not go from the `end` like in the previous test case?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/increment.pass.cpp:82
+ {
+ // forword
+ int buffer[2] = {1, 2};
----------------
Nit: `s/forword/forward/`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/iter_move.pass.cpp:12
+
+// friend constexpr decltype(auto) iter_move(const iterator& i);
+
----------------
Missing the `noexcept` clause (here and in `iter_swap` test).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/iter_move.pass.cpp:36
+
+ static_assert(noexcept(std::ranges::iter_move(std::declval<decltype(v.begin())>())));
+ }
----------------
Wouldn't it be easier to just create a local variable and use it here instead of `declval`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/iter_move.pass.cpp:42
+ std::ranges::zip_view v(throwingMoveRange);
+ static_assert(!noexcept(std::ranges::iter_move(std::declval<decltype(v.begin())>())));
+ }
----------------
Note: there's an `ASSERT_NOT_NOEXCEPT` macro, but using it is completely optional. I don't really have a preference.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/iter_move.pass.cpp:45
+ {
+ IterMoveSwapRange r1{}, r2{};
+ assert(r1.iter_move_called_times == 0);
----------------
`iter_move` must call `ranges::iter_move`, which is a customization point object. Can you please check that your specialization of `iter_move` is found via ADL even if it's in a different namespace (see some other views, e.g. `lazy_split_view`, for an example)?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/iter_swap.pass.cpp:43
+
+ static_assert(noexcept(std::ranges::iter_swap(iter1, iter2)));
+ }
----------------
Check the inverse case as well? (i.e., `!noexcept`)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/iter_swap.pass.cpp:46
+ {
+ IterMoveSwapRange r1, r2;
+ assert(r1.iter_swap_called_times == 0);
----------------
Same comment re. ADL as `iter_move`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/member_types.compile.pass.cpp:38
+template <class T>
+struct diff_type_iter {
+ using iterator_category = std::input_iterator_tag;
----------------
Nit: use `PascalCase` for consistency with names of the other classes in this file?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/member_types.compile.pass.cpp:72
+
+ static_assert(std::is_same_v<Iter::iterator_concept, std::random_access_iterator_tag>);
+ static_assert(std::is_same_v<Iter::iterator_category, std::input_iterator_tag>);
----------------
Can you also test the case when `iterator_concept` is `bidirectional_iterator_tag`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/subscript.pass.cpp:12
+
+// constexpr auto operator[](difference_type __n) const requires
+// all_random_access<_Const, _Views...>
----------------
Note: in synopsis, there is no reason to mangle names -- i.e., please `s/__n/n/`. Can you please check other test files' synopses for similar issues?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/subscript.pass.cpp:13
+// constexpr auto operator[](difference_type __n) const requires
+// all_random_access<_Const, _Views...>
+
----------------
Please check the constraint as well.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/range.concept.compile.pass.cpp:152
+void testConceptTuple() {
+
+ int buffer1[2] = {1, 2};
----------------
Nit: unnecessary blank line.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/range.concept.compile.pass.cpp:157
+
+ // TODO: uncomment all the static_assert once [tuple.tuple] section in P2321R2 is implemented
+ // This is because convertible_to<tuple<int&,int&,int&>&, tuple<int,int,int>> is false without
----------------
Nit: `s/static_assert/static_asserts/` (or `static assertions`).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/range.concept.compile.pass.cpp:306
+};
+static_assert(std::output_iterator<OutputIter, int>);
+
----------------
Can you use the output iterator from `test_iterators.h` instead?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/ctor.other.pass.cpp:34
+
+struct NonSimpleNonCommonConveritbleView : std::ranges::view_base, IntBuffer {
+ using IntBuffer::IntBuffer;
----------------
Nit: `s/Converitble/Convertible/`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/eq.pass.cpp:59
+template <class Iter, class Sent>
+concept EqualComparable = std::invocable < std::equal_to<>,
+const Iter&, const Sent& > ;
----------------
Nit: weird formatting.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/eq.pass.cpp:79
+ using Sentinel = std::ranges::sentinel_t<decltype(v)>;
+ static_assert(EqualComparable<Iter, Sentinel>);
+ }
----------------
Question: isn't this checked by the comparisons above?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/eq.pass.cpp:81
+ }
+ {
+ std::ranges::zip_view v{NonSimpleNonCommon(buffer1), SimpleNonCommon(buffer2), SimpleNonCommon(buffer3)};
----------------
Please add empty lines between the test cases and short comment describing what each test case does.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/minus.pass.cpp:110
+template <class T, class U>
+concept HasMinus = std::invocable < std::minus<>,
+const T&, const U& > ;
----------------
Nit: weird formatting for these concepts (clang-format doesn't support concepts properly, IIRC).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/minus.pass.cpp:120
+
+ {
+ std::ranges::zip_view v{ForwardSizedNonCommon(buffer1)};
----------------
Please add short comments describing what each test case does.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/size.pass.cpp:54
+ assert(std::as_const(v).size() == 8);
+ }
+ {
----------------
Same nit re. empty lines and comments for each test case. Looks like it applies throughout the patch.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/types.h:23
+template <class T>
+struct Buffer {
+ T* buffer_;
----------------
Similar comments to `join_view` -- `BufferView` would be a more appropriate name, see if you can use `std::span` instead.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/types.h:44
+ constexpr int* end()
+ requires(!Simple)
+ {
----------------
Good idea. We have some existing test code that we could simplify this way.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/types.h:45
+ requires(!Simple)
+ {
+ return buffer_ + size_;
----------------
Nit: the opening brace goes on the same line as the function name (applies throughout the file, and possibly in other files as well).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/types.h:309
+using ContiguousNonCommonSized = BasicView<int*, sized_sentinel<int*>>;
+;
+static_assert(std::ranges::contiguous_range<ContiguousNonCommonSized>);
----------------
Nit: extra semicolon.
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