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

Fri Apr 1 01:59:14 PDT 2022

var-const added inline comments.

================
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_);
----------------
var-const wrote:
> var-const wrote:
> > 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).
> Note: filed an LWG issue (will try not to forget to add the number here once it's available).
https://cplusplus.github.io/LWG/issue3686

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/base.pass.cpp:40-43
+static_assert( CanCallBase<std::ranges::lazy_split_view<CopyableView, ForwardView>&>);
+static_assert( CanCallBase<std::ranges::lazy_split_view<CopyableView, ForwardView> const &>);
+static_assert( CanCallBase<std::ranges::lazy_split_view<CopyableView, ForwardView>&&>);
+static_assert( CanCallBase<std::ranges::lazy_split_view<CopyableView, ForwardView> const &&>);
----------------
ldionne wrote:
> IMO it makes more sense to only check the negative cases here, since the positive ones are (or should be) tested "at runtime" below. This comment may apply to other methods you're testing in the patch, too.
In most other cases I fully agree, but here, I did that partially to verify that `CanCallBase` works correctly (e.g. that it doesn't just return `false` all the time) and partially to show which kinds of calls are and aren't valid. I'm absolutely open to remove if you still feel like it's overkill, but just wanted to provide a quick rationale.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp:15
+//             constructible_from<Pattern, single_view<range_value_t<Range>>>
+// constexpr lazy_split_view(Range&& r, range_value_t<Range> e);
+
----------------
ldionne wrote:
> We should be testing that this isn't explicit by using an implicit construction.
Added `test/support/is_explicit_ctr.h` that provides a concept that should work with a constructor with any number of arguments (including a default constructor), and added the check to all constructor tests in this patch. Please let me know if you think it's overkill.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> I really like that you're testing the correctness of the view using a more general approach like this. I think we should also be testing the case where we have an input range and a tiny view (etc...) here, because they use a different branch of  `if constexpr` in various cases, like the `outer_iterator::operator++`.
Added the following:
```
  // In addition to the `(ForwardView, ForwardView)` case, test the `(ForwardView, tiny-range)` and `(InputView,
  // tiny-range)` cases (all of which have unique code paths).
  if constexpr (std::is_same_v<std::remove_reference_t<Separator>, char>) {
    assert(test_function_call(CopyableView(input), ForwardTinyView(separator), expected));
    assert(test_with_piping(CopyableView(input), ForwardTinyView(separator), expected));

    assert(test_function_call(InputView(input), ForwardTinyView(separator), expected));
    assert(test_with_piping(InputView(input), ForwardTinyView(separator), expected));
  }
```

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:316
+
+  // Non-character input.
+  {
----------------
ldionne wrote:
> 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.
Added a vector of maps (since neither is `constexpr`, only checking at runtime).

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/iter_move.pass.cpp:81
+    SplitViewForward v("abc def", " ");
+    auto val = *v.begin();
+
----------------
ldionne wrote:
> I might have called this `chunk` instead, but this is non-blocking.
Went with `segment` if you don't mind (for some reason, I associate `chunk` with raw bytes or similar).

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer.value/begin.pass.cpp:19-21
+constexpr bool operator==(const cpp20_input_iterator<char*>& lhs, const cpp20_input_iterator<char*>& rhs) {
+  return base(lhs) == base(rhs);
+}
----------------
ldionne wrote:
> This shouldn't be necessary -- let's figure out why we need it, and fix the root cause.
Looks like it was a leftover. Thanks for noticing!

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/ctor.copy.pass.cpp:28
+};
+static_assert( IsConstOuterIter<OuterIterConst>);
+
----------------
ldionne wrote:
> It would be nice to run these tests instead.
I removed the positive `is_constructible` test, but I think the rest of this is necessary (or at least useful) to verify the setup. Please let me know if you feel it's excessive.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/small_string.h:29-33
+  constexpr void omit_terminating_null() {
+    if (size_ > 0 && buffer_[size_ - 1] == '\0') {
+      --size_;
+    }
+  }
----------------
ldionne wrote:
> I don't think we want to do this -- I think we want to explicitly test the difference between string literals and char arrays, not hide it in the tests.
Done. This led to changes in `general.pass.cpp` (which I think are ultimately an improvement).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107500