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

Fri Apr 8 13:17:01 PDT 2022

var-const added inline comments.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp:27
+
+constexpr bool test() {
+  // non-const: forward_range<View> && simple-view<View> -> outer-iterator<Const = true>
----------------
philnik wrote:
> I looks like you aren't checking that `begin()` caches the result. Could you add a test for that?
Hmm, this is actually a very interesting question. My understanding was that this class uses `non-propagating-cache` for its "non-copying" behavior, not for actual caching, since the description of `begin` doesn't mention anything about caching, unlike e.g. `filter_view::begin()`. FWIW, neither GCC nor MSVC do caching here (if I'm reading their sources correctly).

However, using caching here only breaks a couple of tests that instantiate `lazy_split_view` with an `InputView` and expect `begin()` to return the beginning of the range after it has already been iterated on, which is likely an incorrect expectation for an input range.

Looking further, it looks like `non-propagating-cache` is defined as being equivalent to `optional` unless specified otherwise. The specification does not "override" `operator=(const T&)`, which means it's equivalent to `optional::operator=(const T&)`, and that does not perform any caching. So if I'm reading this correctly, this line:
```
current_ = ranges::begin(base_);
```
in the Standard does not by itself prescribe caching, and I don't see any additional requirements or remarks to change that.

@tcanens Can you please clarify whether caching behavior is intended in `lazy_split_view::begin()` on the `current_ = ranges::begin(base_);` line?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.default.pass.cpp:41-49
+  {
+    std::ranges::lazy_split_view<CopyableView, ForwardView> v;
+    assert(v.base() == CopyableView());
+  }
+
+  {
+    std::ranges::lazy_split_view<CopyableView, ForwardView> v = {};
----------------
philnik wrote:
> Why do you have effectively the same test twice?
I got this pattern from some other range tests we have. These are two different forms of initialization (default initialization vs. copy initialization), and the latter would fail to compile if the default constructor were `explicit`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp:25
+
+constexpr bool test() {
+  using V = std::ranges::lazy_split_view<std::string_view, std::string_view>;
----------------
philnik wrote:
> It looks like you aren't checking that the constructor doesn't copy the range and pattern. Same for the view, pattern constructor.
Done. This required quite a bit of setup, PTAL.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp:29
+  {
+    decltype(auto) str = "abc def";
+    V v(str, ' ');
----------------
philnik wrote:
> What is `decltype(auto)` here? is it `const char [8]`? Assuming my guess is correct, is it possible to rewrite this as `const char str[] = "abc def"`? I think that would be clearer.
Yes. Removed this occurrence and one other (in `general.pass.cpp`).

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp:40-42
+  struct Empty {};
+  static_assert(!std::is_constructible_v<V, Empty, std::string_view>);
+  static_assert(!std::is_constructible_v<V, std::string_view, Empty>);
----------------
philnik wrote:
> I think you should check here that `lazy_split_view` isn't happy with an `input_range`. I've got a few `ForwardRangeNot*` types in D121964. You can snack those if you want. It also looks like you aren't testing the `constructible_from<V, views::all_t<R>>` requirement. The first `static_assert` will fail at the `forward_range`.
`lazy_split_view` supports `input_range`s as long as `Pattern` is a `tiny-range`. I think these tests are covered by the tests in `range.lazy.split/constraints.compile.pass.cpp`, can you please check and see if you still notice something missing?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.view.pass.cpp:27-31
+  {
+    InputView input = "abc def";
+    std::ranges::lazy_split_view<InputView, ForwardTinyView> v(input, ' ');
+    (void)v;
+  }
----------------
philnik wrote:
> Is there a reason you aren't checking `base()` here?
`InputView` isn't comparable. Do you think it's important enough to define a `ComparableInputView` for this case? (it seems a little not worth it to me, but I'm open to do this if you feel it adds value)

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/assert.equal.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
philnik wrote:
> Shouldn't these kinds of tests be in `libcxx/...`?
Thanks for catching this!

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/ctor.default.pass.cpp:20
+  {
+    [[maybe_unused]] InnerIterForward i;
+  }
----------------
philnik wrote:
> Could you check here that the base iterator is default constructed?
I'm not sure there's a good way of doing that. If the underlying range is an input range, it will crash (`base` tries to access `outer-iterator::parent`, which is null for a default-constructed `outer-iterator`). It will work for a forward range, but my overall impression is that a default-constructed iterator isn't supposed to be used for anything other than reassignment.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107500