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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:118
+
+  template <bool _Const> struct __outer_iterator : __outer_iterator_category<__maybe_const<_Const, _View>> {
+  private:
----------------
The requirement is that `iterator_category` is only present if `_View` is a `forward_range`. I'm concerned that using inheritance here has ABI implications -- is there a better way to achieve this?


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:194
+      } else if constexpr (__tiny_range<_Pattern>) {
+        // TODO(varconst): should be `ranges::find`.
+        __current() = std::find(std::move(__current()), __end, *__pbegin);
----------------
Note: this is significant, though (I think) not enough to block this patch. Because the non-ranges version of the algorithm requires both iterators to have the same type, this will fail for ranges where `end()` returns a sentinel. (same applies to `mismatch` below)


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:305
+    friend constexpr bool operator==(const __inner_iterator& __x, default_sentinel_t) {
+      auto [__pcur, __pend] = ranges::subrange{__x.__i_.__parent_->__pattern_};
+      const auto __end = ranges::end(__x.__i_.__parent_->__base_);
----------------
Note: since there is no check for null on `__parent`, trying to compare a default-constructed `__inner_iterator` would result in a null-pointer dereference. I wonder if this is intentional (the implementation is from the Standard).


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:363
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
+  constexpr auto operator()(_Pattern&& __pattern) const
+  noexcept(is_nothrow_constructible_v<decay_t<_Pattern>, _Pattern>) {
----------------
Please check if this is correct. I got this pattern from `std::views::transform`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctad.compile.pass.cpp:49-52
+  // Note: CTAD from (InputRange, TinyForwardRange) doesn't work -- the deduction guide wraps the pattern in
+  // `views::all_t`, resulting in `views::owning_view<TinyForwardRange>`. That type would never satisfy `tiny-range`
+  // because `views::owning_view` contains a member function `size()` that shadows the static `size()` in
+  // `TinyForwardRange`.
----------------
Just wanted to draw attention to this -- it seems like it could be an oversight.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:32
+      ++next;
+      return *l == '\0' && next == left.end();
+    }
----------------
This is a little hacky, though it works for the purposes of the test. I couldn't think of a way to leverage existing code to replace this function: `std::ranges::equal` is not implemented yet, `std::string` is not `constexpr`, and `std::string_view` cannot be used because split view produces a non-contiguous range.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:229
+      decltype(auto) input = "";
+      constexpr std::array expected = {""sv};
+
----------------
FWIW, I didn't expect some of these results, including the empty string and treatment of several separators in a row. It seems to conform to the Standard implementation, though (please let me know if I'm wrong on this).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:346
+      decltype(auto) input = "abc";
+      constexpr std::array expected = {"a"sv, "b"sv, "c"sv, ""sv};
+
----------------
I'm not sure about the correct behavior here. GCC splits into `["abc", ""]` (https://wandbox.org/permlink/y2Gu0BEfOOP89i9I), and MSVC doesn't have the trailing empty part if I'm reading the code correctly.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/view_interface.pass.cpp:70
+    }
+    // Note: a `lazy_split_view` is never empty.
+  }
----------------
Just drawing attention to this -- even an empty string produces a non-empty output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107500