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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this. I just had a quick look and found some small issues.
I've not looked at the Standard wording of zip. I probably will have time to do a proper review this weekend.



================
Comment at: libcxx/docs/Status/ZipProjects.csv:13
+| `[range.zip.iterator] <https://wg21.link/range.zip.transform>`_, "zip_view::iterator", None, Hui Xie, |In Progress|
+| `[range.zip.sentinel] <https://wg21.link/range.zip.sentinel>`_, "zip_view::sentinel", None, Hui Xie, |In Progress|
 | `[range.zip.transform.view] <https://wg21.link/range.zip.transform.view>`_, "zip_transform_view", "| `zip_transform_view::iterator`
----------------
Is there more work remaining after for these items after this patch is done? If not please change this to `|Complete|`.


================
Comment at: libcxx/include/__ranges/zip_view.h:40
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
----------------
Since you include `__algorithm/min.h` you need to guard against macros using `min`
```
_LIBCPP_PUSH_MACROS
#include <__undef_macros>
```
and at the end of the file
`_LIBCPP_POP_MACROS`
You can look at the `__algorithm/min.h` as an example.


================
Comment at: libcxx/include/__ranges/zip_view.h:63
+_LIBCPP_HIDE_FROM_ABI constexpr auto __tuple_transform(_Fun&& __f, _Tuple&& __tuple) {
+    return apply(
+        [&]<class... _Ts>(_Ts&&... __elements) {
----------------
The same for other calls to `apply`.


================
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_;
----------------
What happens when you remove this?
Is there a clang-format bug report?


================
Comment at: libcxx/include/__ranges/zip_view.h:472
+        return apply(
+            [](auto... ds) {
+                using Diff = common_type_t<range_difference_t<__maybe_const<_OtherConst, _Views>>...>;
----------------



================
Comment at: libcxx/include/__ranges/zip_view.h:473
+            [](auto... ds) {
+                using Diff = common_type_t<range_difference_t<__maybe_const<_OtherConst, _Views>>...>;
+                return (std::min)({Diff(ds)...}, [](auto x, auto y) { return __abs(x) < __abs(y); });
----------------



================
Comment at: libcxx/include/ranges:201
+  // [range.zip], zip view
+  template<input_range... Views>
+    requires (view<Views> && ...) && (sizeof...(Views) > 0)
----------------
Please at `// C++2b` to the new entries. That makes it clear they're added in C++2b.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctad.compile.pass.cpp:31
+void testCTAD() {
+  static_assert(std::same_as<decltype(std::ranges::zip_view(Container{})),
+                             std::ranges::zip_view<std::ranges::owning_view<Container>>>);
----------------
Here you can also use `ASSERT_SAME_TYPE`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/general.pass.cpp:55
+  }
+}
----------------



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:64
+    // clang-format off
+        assert((iter1 <=> iter2) == std::strong_ordering::less);
+        assert((iter1 <=> iter1) == std::strong_ordering::equal);
----------------
The indention is off.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/ctor.default.pass.cpp:58
+  static_assert(test());
+  return 0;
+}
----------------



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/types.h:16
+#include "test_iterators.h"
+#include "test_range.h"
+
----------------
This header should be guarded with `TEST_STD_VER > 20` (or maybe 17) so it won't cause compilation errors when included in older versions. (I know that's unlikely.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122806