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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

I haven't looked at the tests yet. The implementation looks mostly good. Please make sure that you _Uglify all the names.



================
Comment at: libcxx/include/__ranges/zip_view.h:64
+    return apply(
+        [&]<class... _Ts>(_Ts&&... __elements) {
+            return __tuple_or_pair<invoke_result_t<_Fun&, _Ts>...>(
----------------



================
Comment at: libcxx/include/__ranges/zip_view.h:79
+
+template <class _Fun, class _Tuple1, class _Tuple2, size_t... Is>
+_LIBCPP_HIDE_FROM_ABI constexpr auto __tuple_zip_transform(_Fun&& __f,
----------------



================
Comment at: libcxx/include/__ranges/zip_view.h:80-89
+_LIBCPP_HIDE_FROM_ABI constexpr auto __tuple_zip_transform(_Fun&& __f,
+                                                           _Tuple1&& __tuple1,
+                                                           _Tuple2&& __tuple2,
+                                                           index_sequence<Is...>) {
+    return __tuple_or_pair<
+        invoke_result_t<_Fun&, typename tuple_element<Is, remove_cvref_t<_Tuple1>>::type,
+                        typename tuple_element<Is, remove_cvref_t<_Tuple2>>::type>...>{
----------------
I'm not sure, but it may be easier to read if you make the return type part of the declaration and don't put it in the body.


================
Comment at: libcxx/include/__ranges/zip_view.h:127
+template <input_range... _Views>
+    requires(view<_Views>&&...) && (sizeof...(_Views) > 0) 
+class zip_view : public view_interface<zip_view<_Views...>> {
----------------



================
Comment at: libcxx/include/__ranges/zip_view.h:130
+
+    tuple<_Views...> __views_;
+
----------------
It probably doesn't to much, but it doesn't hurt either.


================
Comment at: libcxx/include/__ranges/zip_view.h:142-144
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr explicit zip_view(_Views... __views)
+        : __views_(std::move(__views)...) {}
----------------
The `constexpr`s should be on the same line as `_LIBCPP_HIDE_FROM_ABI`.


================
Comment at: libcxx/include/__ranges/zip_view.h:181
+        return apply(
+            [](auto... sizes) {
+                using CT = make_unsigned_t<common_type_t<decltype(sizes)...>>;
----------------



================
Comment at: libcxx/include/__ranges/zip_view.h:182
+            [](auto... sizes) {
+                using CT = make_unsigned_t<common_type_t<decltype(sizes)...>>;
+                return (std::min)({CT(sizes)...});
----------------



================
Comment at: libcxx/include/__ranges/zip_view.h:183
+                using CT = make_unsigned_t<common_type_t<decltype(sizes)...>>;
+                return (std::min)({CT(sizes)...});
+            },
----------------
This should use `ranges::min`. Why do you have parens around `std::min`?


================
Comment at: libcxx/include/__ranges/zip_view.h:212
+template <bool _Const, class... _Views>
+consteval auto __zip_view_iterator_concept_test() {
+    if constexpr (__zip_all_random_access<_Const, _Views...>) {
----------------
I think a name like `__get_zip_view_iterator_tag` is more descriptive.


================
Comment at: libcxx/include/__ranges/zip_view.h:228
+template <bool _Const, class... _Views>
+requires __zip_all_forward<_Const, _Views...> //
+    struct __zip_view_iterator_category_base<_Const, _Views...> {
----------------
Please don't use `//` to force formatting changes.


================
Comment at: libcxx/include/__ranges/zip_view.h:306
+        __zip_all_random_access<_Const, _Views...> {
+        __tuple_for_each([&]<class _Idx>(_Idx& __i) { __i += iter_difference_t<_Idx>(__x); }, __current_);
+        return *this;
----------------
Shouldn't `_Idx` be `_Iter`?


================
Comment at: libcxx/include/__ranges/zip_view.h:333
+                __tuple_zip_transform(std::equal_to<>(), __x.__current_, __y.__current_);
+            return apply([](auto... bs) { return (bs || ...); }, __it_equals);
+        }
----------------



================
Comment at: libcxx/include/__ranges/zip_view.h:381-383
+        auto __r = __i;
+        __r += __n;
+        return __r;
----------------
I know it's simple, but could you just forward this to the other `operator+`?


================
Comment at: libcxx/include/__ranges/zip_view.h:402
+                return (std::min)({difference_type(ds)...},
+                           [](auto x, auto y) { return __abs(x) < __abs(y); });
+            },
----------------



================
Comment at: libcxx/include/__ranges/zip_view.h:450
+    __sentinel() = default;
+    constexpr __sentinel(__sentinel<!_Const> __i) requires _Const &&
+        (convertible_to<sentinel_t<_Views>, sentinel_t<__maybe_const<_Const, _Views>>> && ...)
----------------
You forgot `_LIBCPP_HIDE_FROM_ABI` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122806