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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

I haven't looked very thoroughly at the tests yet, but it seems like you have tests for copy and move assignment, but none for copy and move construction. Is there a reason for that? Looks pretty good otherwise so far.



================
Comment at: libcxx/include/__ranges/lazy_split_view.h:141
+
+  template <bool _Const> struct __outer_iterator : __outer_iterator_category<__maybe_const<_Const, _View>> {
+  private:
----------------



================
Comment at: libcxx/include/__ranges/lazy_split_view.h:152
+    using _MaybeCurrent = _If<forward_range<_View>, iterator_t<_Base>, __empty_cache>;
+    [[no_unique_address]] _MaybeCurrent __current_ = _MaybeCurrent();
+    bool __trailing_empty_ = false;
----------------
You have to use `_LIBCPP_NO_UNIQUE_ADDRESS`. I think there are other uses of raw `[[no_unique_address]]`.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:192
+      _LIBCPP_HIDE_FROM_ABI
+      constexpr default_sentinel_t end() const { return default_sentinel; }
+    };
----------------
This should be `noexcept`. Please add a regression test.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:226-228
+        ++__current();
+
+      } else if constexpr (__tiny_range<_Pattern>) {
----------------



================
Comment at: libcxx/include/__ranges/lazy_split_view.h:229-230
+      } else if constexpr (__tiny_range<_Pattern>) {
+        // TODO(varconst): use the actual `ranges::find` once it lands.
+        __current() = ranges::__lazy_split_view_find(std::move(__current()), __end, *__pbegin);
+        if (__current() != __end) {
----------------
Same here, `ranges::find` is now available.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:271
+
+    friend constexpr bool operator==(const __outer_iterator& __x, default_sentinel_t) {
+      return __x.__current() == ranges::end(__x.__parent_->__base_) && !__x.__trailing_empty_;
----------------
You missed a `_LIBCPP_HIDE_FROM_ABI` here.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:288
+
+  template <bool _Const> struct __inner_iterator : __inner_iterator_category<__maybe_const<_Const, _View>> {
+  private:
----------------



================
Comment at: libcxx/include/__ranges/lazy_split_view.h:355-356
+        const auto& __cur = __x.__i_.__current();
+        if (__cur == __end) return true;
+        if (__pcur == __pend)  return __x.__incremented_;
+
----------------
Ditto below


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:376
+    friend constexpr decltype(auto) iter_move(const __inner_iterator& __i)
+      noexcept(noexcept(ranges::iter_move(__i.__i_.__current()))) {
+      return ranges::iter_move(__i.__i_.__current());
----------------
Ditto below.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:44-55
+template <input_iterator _Ip, sentinel_for<_Ip> _Sp, class _Tp>
+    requires indirect_binary_predicate<ranges::equal_to, _Ip, const _Tp*>
+_LIBCPP_HIDE_FROM_ABI constexpr
+_Ip __lazy_split_view_find(_Ip __first, _Sp __last, const _Tp& __value) {
+  for (; __first != __last; ++__first) {
+    if (*__first == __value) {
+      break;
----------------
This shouldn't be required anymore. `ranges::find` is landed.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:82
+  _LIBCPP_HIDE_FROM_ABI
+  lazy_split_view()
+    requires default_initializable<_View> && default_initializable<_Pattern> = default;
----------------
Could you add `constexpr`, just for symmetry (and it's marked as `constexpr` in the standard)?


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:89-95
+  template <input_range _Range>
+    requires constructible_from<_View, views::all_t<_Range>> &&
+              constructible_from<_Pattern, single_view<range_value_t<_Range>>>
+  _LIBCPP_HIDE_FROM_ABI
+  constexpr lazy_split_view(_Range&& __r, range_value_t<_Range> __e)
+    : __base_(views::all(std::forward<_Range>(__r)))
+    , __pattern_(ranges::single_view(std::move(__e))) {}
----------------
The `views::single` change should probably be the same, but this is closer to the standards wording. If we don't define `views::single` yet ignore it.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp:42-43
+
+  constexpr std::string_view input = "abc";
+  constexpr std::string_view sep = "a";
+
----------------
Optional, but since you are `using namespace std::string_view_literals` anyways. Or remove it, since you don't seem to use it anywhere.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp:47
+
+  // Test `views::lazy_split(input, sep)`.
+  {
----------------
The ``` aren't really useful IMO, but I don't object strongly.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp:107-108
+    struct X { };
+    auto partial = std::views::lazy_split(X{});
+    (void)partial;
+  }
----------------
Optional


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp:115-123
+    static_assert(!std::is_invocable_v<decltype(std::views::lazy_split)>);
+    static_assert(!std::is_invocable_v<decltype(std::views::lazy_split), SomeView, NotAView>);
+    static_assert(!std::is_invocable_v<decltype(std::views::lazy_split), NotAView, SomeView>);
+    static_assert( std::is_invocable_v<decltype(std::views::lazy_split), SomeView, SomeView>);
+
+    static_assert( CanBePiped<SomeView&,    decltype(std::views::lazy_split)>);
+    static_assert( CanBePiped<char(&)[10],  decltype(std::views::lazy_split)>);
----------------
Could you put these at the top? There is no need for them to be in the function. That has the added benefit that the concept and the tests are next to each other.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/constraints.compile.pass.cpp:35
+// All constraints satisfied (`View` and `Pattern` are forward views).
+namespace test1 {
+
----------------
Nit: it would be nice if these namespace names were a bit more descriptive.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:53-54
+    {
+      std::string_view input = "abc";
+      std::string_view sep = " ";
+      std::ranges::lazy_split_view v(input, sep);
----------------
Again, you could just use `auto input  = "abc"sv`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/ctor.outer_iterator.pass.cpp:36
+template <class Inner, class Outer>
+constexpr void test_impl() {
+  Inner i(Outer{});
----------------
You could name it `test`, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107500