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

Tue Mar 22 12:17:31 PDT 2022

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

Thanks a lot for working on this! This is a monster of a view. It generally looks really good, but I do have a few comments and questions.

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:51
+  requires { typename __require_constant<remove_reference_t<_Range>::size()>; } &&
+  (remove_reference_t<_Range>::size() <= 1);
+
----------------
Can you please add a test with a TinyRange that has a non-constexpr `size()` static function? It should not satisfy `tiny-range`. Same for a size that is `> 1` (if you don't have one already).

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:59-60
+
+  _View __base_ = _View();
+  _Pattern __pattern_ = _Pattern();
+
----------------
We could add `[[no_unique_address]]` here -- I don't foresee this making much of a difference except for a stateless view and pattern (e.g. ones that would generate stuff on the fly), but we don't lose anything.

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:155
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
+    constexpr auto& __current() const noexcept {
+      if constexpr (forward_range<_View>) {
----------------
`const auto&`? This is mostly for the reader, since it doesn't change the actual meaning.

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:210
+      if (__current() == __end) {
+        __trailing_empty_ = false;
+        return *this;
----------------
I do not understand the purpose of `__trailing_empty` (comment for the spec, not for this specific implementation).

What tests start failing if you stop setting `__trailing_empty_` to false or true below? Or if you set it to the wrong value (e.g. `true` here)?

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:215
+      const auto [__pbegin, __pend] = ranges::subrange{__parent_->__pattern_};
+      if (__pbegin == __pend) {
+        ++__current();
----------------
`// Empty pattern: split on every element in the input range`

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:218
+
+      } else if constexpr (__tiny_range<_Pattern>) {
+        __current() = ranges::find(std::move(__current()), __end, *__pbegin);
----------------
`// One-element pattern: use ranges::find because we can`?

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:221
+        if (__current() != __end) {
+          ++__current();
+          if (__current() == __end)
----------------
Perhaps we could add a comment like `// make sure we point to after the separator we just found` or something along those lines -- I was initially surprised that we incremented again after a successful `ranges::find`.

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:226
+
+      } else {
+        do {
----------------
`// General case for n-element pattern`

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:228
+        do {
+          const auto [__b, __p] = std::ranges::mismatch(__current(), __end, __pbegin, __pend);
+          if (__p == __pend) {
----------------
(perhaps elsewhere too)

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:234
+            }
+            break; // The pattern matched; skip it.
+          }
----------------
I am wondering what happens if we do something like this: `split("hellofoofooworld"sv, "foo"sv)`. I would assume we get `{"hello"sv, ""sv, "world"sv}`, however my understanding is that we'll get `{"hello"sv, "fooworld"sv}` with the current implementation. Can you confirm what happens, and make sure that it's tested?

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:250
+      } else {
+        ++*this;
+      }
----------------
Do you test that this returns `void` on non-`forward_range`s?

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:329
+      } else {
+        ++*this;
+      }
----------------
Same comment about checking for `void` return here.

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:357
+          return true;
+        if (__pcur == __pend)
+          return __x.__incremented_;
----------------
Do we have tests with an empty pattern that do not exercise the tiny-range code path?

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:360
+
+        do {
+          if (*__cur != *__pcur)
----------------
Do you have a test that would fail if `__x.__incremented` was always false (or true) for a non-tiny-range pattern?

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:71-72
+
+  _View __base_ = _View();
+  _Pattern __pattern_ = _Pattern();
+
----------------
`[[no_unique_address]]`?

================
Comment at: libcxx/include/__ranges/lazy_split_view.h:40
+
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+
----------------
var-const wrote:
> Quuxplusone wrote:
> > `#if !defined(_LIBCPP_HAS_NO_CONCEPTS) && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)`
> > (and likewise on the `#endif` of course)
> > 
> I //think// that now that libc++ no longer supports older compilers, the `_LIBCPP_HAS_NO_CONCEPTS` is unnecessary; but it's a C++20 feature, so it seems like the guard should be `#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)`.
The guard should be `#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp:111
+
+  // Test that one can call std::views::filter with arbitrary stuff, as long as we
+  // don't try to actually complete the call by passing it a range.
----------------
Also consider searching in this patch in case there are other incorrect references.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/base.pass.cpp:74
+    {
+      constexpr View str("abc def");
+      std::ranges::lazy_split_view<View, std::string_view> v(str, " ");
----------------
I don't think this should be `constexpr` (applies elsewhere too).

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/base.pass.cpp:77
+
+      std::same_as<View> auto result = v.base();
+      assert(result == str);
----------------
It would be better to test the result of functions with `ASSERT_SAME_TYPE` when it's impossible to test it with `same_as`, which is (almost) the case for functions that return references. Because if `v.base()` returned `View const&` instead of `View` (as it should), we wouldn't catch it right now.

As you just said in the live review, I think you could also switch to `decltype(auto)` instead to catch this.

I suspect this comment applies to other functions as well.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:46-49
+    if (expected_it == expected.end()) return false;
+
+    if (SmallString(e) != *expected_it) return false;
+    ++expected_it;
----------------
Non-blocking suggestion. Initially I thought the `++expected_it` was tied to the `if` because of the indentation.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:237
+  }
+
+  // Empty separator.
----------------
Can you please make sure you have tests for all the corner cases you showed me (the ones we want to take to the reflector)?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:316
+
+  // Non-character input.
+  {
----------------
Can we also add a test case with *really* non-trivial "characters", i.e. splitting a sequence of maps or something like that. It's stretching it, but I just want to confirm that it works, since it's a "corner" case.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107500