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

[Hui via Phabricator via libcxx-commits]

huixie90 marked 26 inline comments as done.
huixie90 added inline comments.


================
Comment at: libcxx/include/__ranges/zip_view.h:142-144
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr explicit zip_view(_Views... __views)
+        : __views_(std::move(__views)...) {}
----------------
philnik wrote:
> The `constexpr`s should be on the same line as `_LIBCPP_HIDE_FROM_ABI`.
I took a look at several other views (transform, single, subrange, take, join). they all have `_LIBCPP_HIDE_FROM_ABI` on a separate line.

A side question. why don't we put `_LIBCPP_HIDE_FROM_ABI` put on the class declaration so we can avoid putting `_LIBCPP_HIDE_FROM_ABI` on all member functions


================
Comment at: libcxx/include/__ranges/zip_view.h:183
+                using CT = make_unsigned_t<common_type_t<decltype(sizes)...>>;
+                return (std::min)({CT(sizes)...});
+            },
----------------
philnik wrote:
> This should use `ranges::min`. Why do you have parens around `std::min`?
both `std::min` and `ranges::min` would work as they both accepts `initializer_list`. For some reason, I thought `ranges::min` wasn't implemented in libcxx but I was wrong I found it now. I switched to use `ranges::min`. The reason I used to add parens was because that prevents macro expansion in case `min` was a macro. But anyway, Mark suggested a to use the `_LIBCPP_PUSH_MACROS`. So I used that technique instead


================
Comment at: libcxx/include/__ranges/zip_view.h:365
+        (three_way_comparable<iterator_t<__maybe_const<_Const, _Views>>> && ...) {
+        // clang-format off
+        return __x.__current_ <=> __y.__current_;
----------------
huixie90 wrote:
> Mordante wrote:
> > What happens when you remove this?
> > Is there a clang-format bug report?
> A white space is added between the spaceship  `<= >`. I think my clang format version is quite recent. Not sure if this is known issue
removed. I accidentally used a different .clang-format config file, which indented it as `<= >`. If I use the .clang-format in the llvm root, it will indent it as normal `<=>`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122806