[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 reviewer: var-const.
Mordante added a comment.

In general SGTM some small issues. However I'm not too familiar with our ranges implementation so I like somebody more familiar with ranges to approve.



================
Comment at: libcxx/include/__ranges/zip_view.h:115
+template <class _Tp>
+_LIBCPP_HIDE_FROM_ABI constexpr auto __abs(_Tp t) {
+  return t < 0 ? -t : t;
----------------
Please don't use `auto` here.


================
Comment at: libcxx/include/__ranges/zip_view.h:40
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
----------------
huixie90 wrote:
> Mordante wrote:
> > 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.
> I did notice this because the macro unit test failed and i worked around it by adding additional parentheses `(std::min)(arg…)` . The additional parenthesis prevent macro from expanding.  Is this considered bad practice? 
The push/pop method it the normal way we handle this in libc++. (In fact in the past all our header had this guard. Only recently we changed to only include it when needed.)


================
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:
> 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 `<=>`
Great! It probably is due to whether clang-format is set to c++20 or another version.


================
Comment at: libcxx/include/ranges:201
+  // [range.zip], zip view
+  template<input_range... Views>
+    requires (view<Views> && ...) && (sizeof...(Views) > 0)
----------------
Mordante wrote:
> Please at `// C++2b` to the new entries. That makes it clear they're added in C++2b.
Please add this to all individual items. When C++26 adds new entries between these it can become unclear which version of C++ added what.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/begin.pass.cpp:54
+  static_assert(test());
+}
----------------
Please make sure all main functions end with `return 0;`. Some of our supported platforms require this.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/subscript.pass.cpp:23
+  int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
+  std::ranges::zip_view v(SizedRandomAccessView{buffer}, std::views::iota(0));
+  auto it = v.begin();
----------------
In general it would be good when you test an iterator to also the compatible iterators. So in this case testing both random access and contiguous. The same applies the other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122806