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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

Thanks for working on this! Please make sure you did everything on the pre-commit check list from https://libcxx.llvm.org/Contributing.html (in particular updating the status papers). I'll have a closer look later.



================
Comment at: libcxx/include/__ranges/zip_view.h:27
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#pragma GCC system_header
+#endif
----------------



================
Comment at: libcxx/include/__ranges/zip_view.h:36-39
+template <class... _Rs>
+concept __zip_is_common = (sizeof...(_Rs) == 1 && (common_range<_Rs> && ...)) ||
+                          (!(bidirectional_range<_Rs> && ...) && (common_range<_Rs> && ...)) ||
+                          ((random_access_range<_Rs> && ...) && (sized_range<_Rs> && ...));
----------------
Please use `_Ranges` instead of `_Rs`. Same for the other short template names.


================
Comment at: libcxx/include/__ranges/zip_view.h:41-42
+
+template <typename _T, typename _U>
+auto __tuple_or_pair_test() -> pair<_T, _U>;
+
----------------



================
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:
> 
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>;
```
?


================
Comment at: libcxx/include/__ranges/zip_view.h:44-45
+
+template <typename... _Ts>
+requires(sizeof...(_Ts) != 2) auto __tuple_or_pair_test() -> tuple<_Ts...>;
+
----------------



================
Comment at: libcxx/include/__ranges/zip_view.h:55
+            return __tuple_or_pair<invoke_result_t<_F&, _Ts>...>(
+                invoke(__f, static_cast<_Ts&&>(__elements))...);
+        },
----------------
You have to qualify all the function calls too.


================
Comment at: libcxx/include/__ranges/zip_view.h:57
+        },
+        static_cast<_Tuple&&>(__tuple));
+}
----------------
Please use `std::forward`.


================
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,
----------------
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?


================
Comment at: libcxx/include/__ranges/zip_view.h:462
+                using D = common_type_t<range_difference_t<__maybe_const<_OtherConst, _Views>>...>;
+                return min({D(ds)...}, [](auto x, auto y) { return __abs(x) < __abs(y); });
+            },
----------------
Please add a TODO to remove `__abs` once P0533R9 is implemented.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctad.compile.pass.cpp:12-13
+
+// template <class... _Rs>
+// zip_view(_Rs&&...) -> zip_view<views::all_t<_Rs>...>;
+
----------------
These names shouldn't be mangled.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/general.pass.cpp:36
+  {
+    using namespace std::string_literals;
+    std::vector v{1, 2, 3, 4};
----------------
What is that for?


================
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>);
----------------
Why are these `[[maybe_unused]]`? Don't they have a defined state after default construction?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/decrement.pass.cpp:41-42
+
+    ASSERT_SAME_TYPE(decltype(--it), Iter&);
+    --it;
+    assert(&(std::get<0>(*it)) == &(a[2]));
----------------
You should check here that the iterator itself is returned (i.e. has the same address). Same for `operator++`


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/decrement.pass.cpp:68
+}
\ No newline at end of file

----------------
Could you add newlines at the end of the files?


================
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 = {};
+
----------------
What is the difference here?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/types.h:1
+#ifndef TEST_STD_RANGES_RANGE_ADAPTORS_RANGE_ZIP_TYPES_H
+#define TEST_STD_RANGES_RANGE_ADAPTORS_RANGE_ZIP_TYPES_H
----------------
Please add the license header here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122806