[libcxx-commits] [PATCH] D122806: add zip_view and views::zip for C++23

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/__ranges/zip_view.h:41-48
+template <typename _T, typename _U>
+auto __tuple_or_pair_test() -> pair<_T, _U>;
+
+template <typename... _Ts>
+requires(sizeof...(_Ts) != 2) auto __tuple_or_pair_test() -> tuple<_Ts...>;
+
+template <class... _Ts>
----------------
philnik wrote:
> philnik wrote:
> > 
> Couldn't you just do
> ```
> template <class... _Types> requires (sizeof...(_Type) != 2)
> using __tuple_or_pair = tuple<_Types...>;
> 
> template <class _T1, class _T2>
> using __tuple_or_pair = pair<_T1, _T2>;
> ```
> ?
I don't think so
https://godbolt.org/z/br3jqGaTr


================
Comment at: libcxx/include/__ranges/zip_view.h:69
+template <class _F, class _Tuple1, class _Tuple2, size_t... Is>
+_LIBCPP_HIDE_FROM_ABI constexpr auto __tuple_zip_transform(_F&& __f,
+                                                           _Tuple1&& __tuple1,
----------------
philnik wrote:
> Is there a reason you go with a different name than the standard? And why is this so much more complicated than the version in the standard?
I used the same name and implementations for `tuple-for-each` and `tuple-transform`

The `tuple-zip-transform` and `tuple-zip-for-each` are just some additional utilities to implement something else in the standard:

> Let D be the return type. Let DIST(i) be D(std::get<i>(x.current_) - std::get<i>(y.end_)).
> Returns: The value with the smallest absolute value among DIST(n) for all integers 0 ≤ n < sizeof...(Views).

The above example needs to zip two tuples and then do a transform (effectively zip_transform, but on two tuples). The standard doesn't give the implmentation


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/ctor.default.pass.cpp:44
+    using It = zip_iter<Default>;
+    [[maybe_unused]] It it;
+    static_assert(std::default_initializable<It>);
----------------
philnik wrote:
> Why are these `[[maybe_unused]]`? Don't they have a defined state after default construction?
the variable `it` is unused. it would fail with the build that has `-Wunused -Werror`


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/ctor.default.pass.cpp:23-24
+  using Sentinel = std::ranges::sentinel_t<R>;
+  [[maybe_unused]] Sentinel st1;
+  [[maybe_unused]] Sentinel st2 = {};
+
----------------
philnik wrote:
> What is the difference here?
The latter wouldn't work if the default constructor were marked `explicit`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122806/new/

https://reviews.llvm.org/D122806