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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:82
+  _LIBCPP_HIDE_FROM_ABI
+  lazy_split_view()
+    requires default_initializable<_View> && default_initializable<_Pattern> = default;
----------------
var-const wrote:
> philnik wrote:
> > Could you add `constexpr`, just for symmetry (and it's marked as `constexpr` in the standard)?
> Hmm, I don't think it's marked `constexpr` here: https://eel.is/c++draft/range.lazy.split.view Am I missing something?
Hmm, I probably looked at the wrong constructor. I'd still like to see it for symmetry with the rest of the constructors.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:355-356
+        const auto& __cur = __x.__i_.__current();
+        if (__cur == __end) return true;
+        if (__pcur == __pend)  return __x.__incremented_;
+
----------------
var-const wrote:
> philnik wrote:
> > Ditto below
> This formatting is copied from the Standard. I'm slightly in favor of keeping it consistent with the Standard (or otherwise I'd also add braces). Let me know if you feel strongly about this.
We aren't using the formatting from the standard in most of the code and I find the one-line ifs much harder to read. For this review it's probably a //bit// easier, but for anybody looking at this code later it will be hard to read.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp:112
+}
+
+int main(int, char**) {
----------------
You aren't checking `forward_range<V> && simple-view<V> && !simpile-view<Pattern>`, or is that the `tiny-range` test?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/constraints.compile.pass.cpp:35
+// All constraints satisfied (`View` and `Pattern` are forward views).
+namespace test1 {
+
----------------
var-const wrote:
> philnik wrote:
> > Nit: it would be nice if these namespace names were a bit more descriptive.
> Hmm, I got this idea from a pending patch, but I presumed that these namespaces were a nice way to imitate the usual `{}` braces we use to separate test cases. From that perspective, the fact that they're effectively nameless seemed like an advantage, since regular scopes are nameless as well. The comment should describe the purpose of each test case anyway. I'm not strongly opposed, though -- let me know what you think.
If you want them to be effectively nameless, why not make them actually nameless? Since you are `static_assert`ing there shouldn't be any unused things. Then I wouldn't first look at the name to find out what this section is about, but directly look for the comment.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctad.compile.pass.cpp:60
+  test<InputRange, bool, std::ranges::views::all_t<InputRange>>();
+
+  // Note: CTAD from (InputRange, TinyForwardRange) doesn't work -- the deduction guide wraps the pattern in
----------------
I'm not really happy with the CTAD test, because you are never checking that the second template parameter has been deduced correctly. Please change that.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.default.pass.cpp:33
+static_assert(!std::is_default_constructible_v<std::ranges::lazy_split_view<NoDefaultCtorForwardView, ForwardView>>);
+static_assert(!std::is_default_constructible_v<std::ranges::lazy_split_view<ForwardView, NoDefaultCtorForwardView>>);
+
----------------
Just for good measure, could you add `NoDefaultCtorForwardView, NoDefaultCtorForwardView`? Same for the `noexcept` test.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:23-31
+template <class T, size_t N, class Separator, class U, size_t M>
+constexpr bool test_function_call(const T (&input)[N], Separator separator, std::array<U, M> expected) {
+  std::ranges::lazy_split_view v(input, separator);
+
+  return std::equal(v.begin(), v.end(), expected.begin(), expected.end(),
+      [](const auto& split_value, const auto& expected_value) {
+        return SmallString(split_value) == expected_value;
----------------
Nit: I think the usual pattern is to have the function return `void` and assert inside the function. Same with `test_with_piping`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:67
+    {
+      [[maybe_unused]] std::ranges::lazy_split_view v("abc"sv, " "sv);
+    }
----------------
Nit: Could you put the strings into variables and then move them? That would make the pattern more obvious.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/ctor.outer_iterator.pass.cpp:36
+template <class Inner, class Outer>
+constexpr void test_impl() {
+  Inner i(Outer{});
----------------
var-const wrote:
> philnik wrote:
> > You could name it `test`, right?
> Making a non-template and template functions share the same name seemed unnecessarily confusing when I was writing this. Do you think it would be better?
The template is only called in `test()`, so I don't think there is much opportunity for confusion, but I don't really care. Both are readable IMO.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/equal.pass.cpp:73
+  }
+
+  return true;
----------------
Please also check that two default constructed iterators compare equal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107500