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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik accepted this revision.
philnik added a comment.
This revision is now accepted and ready to land.

Other than moving the libc++-specific tests in the `libcxx/` subdirectory all the comments are very nit-picky, so this LGTM!



================
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>
----------------
I looks like you aren't checking that `begin()` caches the result. Could you add a test for that?


================
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 = {};
----------------
Why do you have effectively the same test twice?


================
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>;
----------------
It looks like you aren't checking that the constructor doesn't copy the range and pattern. Same for the view, pattern constructor.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp:29
+  {
+    decltype(auto) str = "abc def";
+    V v(str, ' ');
----------------
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.


================
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>);
----------------
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`.


================
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;
+  }
----------------
Is there a reason you aren't checking `base()` here?


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


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/ctor.default.pass.cpp:20
+  {
+    [[maybe_unused]] InnerIterForward i;
+  }
----------------
Could you check here that the base iterator is default constructed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107500