[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
Fri Apr 22 04:37:49 PDT 2022
huixie90 added inline comments.
================
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,
----------------
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 {.....}`).
================
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...> {
+
----------------
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?
================
Comment at: libcxx/include/__ranges/zip_view.h:286
+ requires __zip_all_forward<_Const, _Views...>
+ {
+ auto __tmp = *this;
----------------
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
================
Comment at: libcxx/include/__ranges/zip_view.h:394
+ {
+ return __i + __n;
+ }
----------------
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?)
================
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>>&>())) &&
----------------
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
================
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)...);
+ }
----------------
var-const wrote:
> 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`).
good catch. added unit test to lock down this. for some reason, I relied on the user defined CTAD (which has the all_t thing, but forgot about the implicit CTAD from copy constructors which don't have 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> && ...);
+
----------------
var-const wrote:
> 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?
I don't think it is possible.
To make non-const overload not exist, all `Views` have to model `simple-view`. `simple-view` implies `range<const View>`. so the const overload must exist in that case.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/cpo.pass.cpp:12
+
+// std::views::zip
+
----------------
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]).
================
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>>);
----------------
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.
================
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>);
+
----------------
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.
================
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; }
+};
----------------
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
================
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&);
+
----------------
var-const wrote:
> Is this declaration necessary? It seems to go against the purpose of the test, which IIUC verifies that having just `operator==` is sufficient.
This particular test point was suggested by @philnik . His point is that testing "zip_iterator is never calling underlying iterator's >, >=, <=, !=
because the spec says zip_iterator's >= is calling zip_iterator's (not less_than). So tested this way: 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 runtime tests and non-constant expression for compile time test.
added comments to explain this
================
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&);
----------------
var-const wrote:
> Question: do these operators have to be declared?
This particular test point was suggested by @philnik . His point is that testing "zip_iterator is never calling underlying iterator's >, >=, <=, !=
because the spec says zip_iterator's >= is calling zip_iterator's (not less_than). So tested this way: 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 runtime tests and non-constant expression for compile time test
added comments to explain this
================
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>>);
----------------
var-const wrote:
> Similar question to `zip_view`'s default constructor -- where does this constraint come from?
this comes implicitly from the tuple's constructor
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/decrement.pass.cpp:63
+
+ ++it;
+ ++it;
----------------
var-const wrote:
> What's the reason to not go from the `end` like in the previous test case?
In this case, zip_view is not a `common_range`, `end` returns a sentinel which can't be decremented
================
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);
----------------
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.
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