[libcxx-commits] [PATCH] D121204: [libc++][ranges] Implement `lazy_split_view`.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I had a look at the code, however I feel I'm not the most qualified person to review this code. So I left comment, but I leave the approval to somebody more familiar with this part of ranges.



================
Comment at: libcxx/include/__ranges/lazy_split_view.h:9
+//===----------------------------------------------------------------------===//
+#ifndef _LIBCPP___RANGES_LAZY_SPLIT_VIEW_H
+#define _LIBCPP___RANGES_LAZY_SPLIT_VIEW_H
----------------
Nit:  we usually have a blank line before the include guard.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:67
+public:
+  lazy_split_view()
+    requires default_initializable<_View> && default_initializable<_Pattern> = default;
----------------
This function should be marked `_LIBCPP_HIDE_FROM_ABI`, similar for most functions in this patch.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:224
+        return __tmp;
+      } else {
+        ++*this;
----------------
The LLVM coding-style prefers to omit curly-braces when there's only one statement in the branch. 
(I noticed the same in other parts of this review.)


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:260
+    using iterator_concept = typename __outer_iterator<_Const>::iterator_concept;
+    using value_type = range_value_t<_Base>;
+    using difference_type = range_difference_t<_Base>;
----------------
I miss the public `iterator_category`.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:310
+        const auto& __cur = __x.__i_.__current();
+        if (__cur == __end) return true;
+        if (__pcur == __pend)  return __x.__incremented_;
----------------
Please move the return on its own line, since that's the LLVM style. (I know this matches the Standard wording.)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp:10
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
----------------
This is no longer required, same for the other headers. (It was when you uploaded the patch.)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp:26
+template <class V, class P, class R, class D>
+concept CanConstruct = requires(R&& r, D&& d) {
+  std::ranges::lazy_split_view<V, P>(std::forward<R>(r), std::move(d));
----------------
This looks odd `D&& d` which is moved instead of forwarded. When this is intended please add comment explaining why this is intended.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:75
+      std::ranges::lazy_split_view v(input, sep);
+      (void)v;
+    }
----------------
How do you feel about using ``[[maybe_unused]]` instead?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:340
+
+  // Empty separator.
+  {
----------------
Is '\0' a valid separator?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:393
+    }
+  }
+
----------------
I think it would be nice to also test with `wchar_t`, `char(8|16|32)_t`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/ctor.copy.pass.cpp:15
+
+int main(int, char**) {
+  return 0;
----------------
What is the purpose of these empty tests? When intended please add comment why it's intended.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/types.h:103
+
+    TEST_CONSTEXPR almost_forward_iterator() : it_() {}
+    TEST_CONSTEXPR explicit almost_forward_iterator(It it) : it_(it) {}
----------------
This looks copy pasted, please replace `TEST_CONSTEXPR.*` with `constexpr`.
(I assume it's not possible to use the original.)


================
Comment at: libcxx/test/support/test_iterators.h:519
 };
+#ifndef _LIBCPP_HAS_NO_CONCEPTS
+  static_assert(std::input_iterator<cpp20_input_iterator<int*>>);
----------------
This is no longer required. (It was when you uploaded the patch.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121204