[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
Fri Apr 22 22:01:55 PDT 2022


var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Almost LGTM, just a few nits.



================
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,
----------------
huixie90 wrote:
> var-const wrote:
> > Is specifying the complicated return type necessary here (i.e., could this function get away with just `auto`)?
> I guess I'd have to spell this type in the return statement (currently it is just `return {.....}`).
I mean, could you rely on CTAD? To be clear, it's just a question -- perhaps CTAD won't work here.


================
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...> {
+
----------------
huixie90 wrote:
> var-const wrote:
> > 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)
> AFAIK, doing anything with iterator of singular value would result in undefined behaviour (except for very few operations). one question is can we take advantage of the undefined behaviour and not do anything here?
> 
> One thing we could do is to value initialise all the underlying iterators. but what if underlying iterators are not default constructible?
Perhaps we should test that the iterator is reassignable/destroyable, but perhaps it's overkill. Let's check with Louis @ldionne.


================
Comment at: libcxx/include/__ranges/zip_view.h:286
+    requires __zip_all_forward<_Const, _Views...>
+  {
+    auto __tmp = *this;
----------------
huixie90 wrote:
> var-const wrote:
> > Nit: a few of these functions should be formatted so that the opening brace is on the same line as the function name.
> it is done by clang-format. happy to format manually. 
> 
> but imo it would be great to come up with a clang-format file that everyone is sort of OK-ish with it. It would save everyone's  time
I fully agree, being able to use `clang-format` would be great (though note that it's not fully straightforward on our code base).


================
Comment at: libcxx/include/__ranges/zip_view.h:394
+  {
+    return __i + __n;
+  }
----------------
huixie90 wrote:
> var-const wrote:
> > Why is this implementation different from the overload above (and from the Standard)?
> isn't this just delegating to the above? (thus same as above?)
It's a little easier to compare when things are close to the Standard text (unless, of course, there's a reason to diverge), but it's a really small thing -- we can keep as is.


================
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>>&>())) &&
----------------
huixie90 wrote:
> var-const wrote:
> > 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)`.
> > ```
> I think so. In the standard, it basically says all the `iter_swap(std::get<i>(l.current_), std::get<i>(r.current_)` are `noexcept`. `std::get` itself is always noexcept, so the standard basically equivalent to all the  `ranges::iter_swap` are `noexcept`.
> 
> Here the implementation is checking the same thing
Sounds reasonable, but I'd ask Louis @ldionne to double-check just in case.


================
Comment at: libcxx/include/__ranges/zip_view.h:427
+
+  // hidden friend cannot access private member of iterator because
+  // they are friends of friends
----------------
Very optional: this comment can fit in one line.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/begin.pass.cpp:51
+  {
+    // all underlying iterators should at begin position
+    std::ranges::zip_view v(SizedRandomAccessView{buffer}, std::views::iota(0), std::ranges::single_view(2.));
----------------
A word seems to be missing -- "should _be_ at the begin position"?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/cpo.pass.cpp:12
+
+// std::views::zip
+
----------------
huixie90 wrote:
> var-const wrote:
> > 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.
> `views::zip` is not an "adaptor". It is only a CPO. So it doesn't support pipe operation.
> Therefore the test is not named as `adaptor.pass.cpp` but `cpo.pass.cpp`
> 
> > The name views​::​zip denotes a customization point object ([customization.point.object]).
> 
Ah, makes sense, never mind.


================
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>>);
----------------
huixie90 wrote:
> var-const wrote:
> > 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?
> I had the same puzzle before I implemented this and asked sg9.  Why there is no "requires (default_initializable<iterator_t<maybe-const<const, views>>> && ...)
> 
> The answer is that it is implicitly required by the `tuple`'s constructor. if any of the iterators are not default constructible, zip_iterator's `=default` would be implicitly deleted.
Because it's not obvious, can you please add this as a comment?


================
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>);
+
----------------
huixie90 wrote:
> var-const wrote:
> > Wouldn't `std::convertible_to` work?
> My original thought was `convertible_to` doesn't work for multiple arguments but `zip_view` constructor can take multiple arguments. so i implemented a multi-args version.
> But then I forgot to test the multi-args case.
Ah, right (you might also want to check that the default constructor is `explicit`, though it starts going into the overkill territory).


================
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; }
+};
----------------
huixie90 wrote:
> var-const wrote:
> > Seems like this could be UB? Consider making `moves` a one-element array.
> I don't think this is UB. AFAIK, this is how `single_view` is implemented
> 
> https://eel.is/c++draft/range.single#view-5
Thanks for pointing it out -- it made me find [[ https://stackoverflow.com/a/48322944 | this post ]] that quotes the relevant part of the Standard (updated to the current draft):

> When an expression `J` that has integral type is added to... an expression `P` of pointer type...
>
> <...>
>
> (4.2) Otherwise, if `P` points to an array element `i` of an array object `x` with `n` elements (`[dcl.array]`),70 the expressions `P + J` and `J + P`... point to the (possibly-hypothetical) array element  `i + j` of `x` if  `0 ≤ i+j ≤ n`...
>
><...>
>
>70) As specified in `[basic.compound]`, an object that is not an array element is considered to belong to a single-element array for this purpose and a pointer past the last element of an array of `n` elements is considered to be equivalent to a pointer to a hypothetical array element `n` for this purpose.


================
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();
----------------
var-const wrote:
> 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).
(By the way, ignoring optional comments is fine -- that's why they're optional, after all -- but if you decide not to implement an optional suggestion, can you please add a comment to that effect? This would help me understand whether this is something you plan to come back to later or something you decided to push back on. Once again, pushing back is fine -- I just want to make sure I understand the state of the patch, i.e. whether all comments are addressed one way or the other)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/arithmetic.pass.cpp:19
+// All the arithmetic operators have the constraint `requires all-random-access<Const, Views...>;`,
+// except `operator-(x, y)` which has the constraint 
+//    `requires (sized_­sentinel_­for<iterator_t<maybe-const<Const, Views>>,
----------------
Nit: I think the meaning would be a little more obvious if `s/which/which instead/`. (I know the current text is my suggestion)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/arithmetic.pass.cpp:101
+  {
+    // in this case sentinel is computed by getting each of the underlying sentinel, so the distance
+    // between begin and end for each of the underlying iterators can be different
----------------
Nit: with the new sentence structure, I think this should be `sentinels`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:35
+// Declare all the operations >, >=, <= etc to make it satisfy random_access_iterator concept,
+// but not define them. If the zip iterator's >,>=, <=, etc isn't implemented as the way standard defined,
+// but calling underlying iterator's >,>=,<=, we will get a linker error for the runtime tests and
----------------
Some nits:
- `s/as/in`;
- consider `in the way defined by the standard`;
- I think there should be no comma between `defined` and `but`;
- consider `s/but/but instead/`;
- (ultranit) please add a full stop at the end of the sentence.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:129
+
+constexpr void inequalityOpeartorsDoNotExistTest(auto&& iter1, auto&& iter2) {
+  using Iter1 = decltype(iter1);
----------------
Nit: `s/Opeartors/Operators/`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:238
+    using Iter = decltype(r.begin());
+    static_assert(!std::invocable<std::equal_to<>, Iter, Iter>);
+  }
----------------
Should it also call `inequalityOperatorsDoNotExistTest`?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/ctor.other.pass.cpp:47
+
+    static_assert(!std::is_same_v<decltype(iter1), decltype(iter2)>);
+
----------------
Is it possible to also add a `static_assert` to show that the `convertible_­to` constraint is not satisfied in this case? (Only if it's straightforward, it's basically to "test the test" and to make it a little more obvious what exactly it's testing)


================
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);
----------------
huixie90 wrote:
> var-const wrote:
> > `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)?
> happy to add the extra test.
> Just FYI, this is somehow implicitly tested by the #include order. we include <ranges> before the "types.h". If it were not using ADL, it couldn't find our test iterator's iter_move because normal lookup requires the declaration to be seen first.
Thanks for adding the test! Relying implicitly on the include order would be too subtle, IMO.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/member_types.compile.pass.cpp:69
+  {
+    // 2 ranges shoud have pair value_type
+    // random_access_iterator_tag
----------------
Nit: `s/shoud/should/`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/member_types.compile.pass.cpp:69
+  {
+    // 2 ranges shoud have pair value_type
+    // random_access_iterator_tag
----------------
var-const wrote:
> Nit: `s/shoud/should/`.
Nit: probably `s/ranges/views/`? That's the name of the template argument / constructor argument.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/member_types.compile.pass.cpp:146
+  {
+    // difference_type multiple ranges should be the common type
+    std::ranges::zip_view v{DiffTypeRange<intptr_t>{}, DiffTypeRange<std::ptrdiff_t>{}};
----------------
Nit: `s/multiple/of multiple` (here and below).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/eq.pass.cpp:131
+  {
+    // underlying const/non-const sentinel cannot be compared with non-const/const iterator
+    std::ranges::zip_view v{ComparableView(buffer1), ConstIncompatibleView{}};
----------------
Thanks for adding all the comments! Much easier to skim through now.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/minus.pass.cpp:159
+    // const imcompatible:
+    // underlying const sentinels cannot minus underlying iterators
+    // underlying sentinels cannot minus underlying const iterators
----------------
Optional nit: `s/minus/subtract/`.


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