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

Wed Mar 16 22:09:50 PDT 2022

var-const 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;
----------------
philnik wrote:
> 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.
For interfaces, I'd prefer to stay consistent with the Standard unless there's a strong reason to diverge. It seems like the simplest way to ensure we don't diverge by accident.

================
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_;
+
----------------
philnik wrote:
> 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.
Done. It is a matter of preference -- I personally find the one-line ifs quite readable and the LLVM style guide-favored two-line ifs without braces very hard to parse (which is to say that this function is less readable for me now).

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp:112
+}
+
+int main(int, char**) {
----------------
philnik wrote:
> You aren't checking `forward_range<V> && simple-view<V> && !simpile-view<Pattern>`, or is that the `tiny-range` test?
Added. The problem with this test is that I'm not sure how to observe the difference. If `View` returns different iterators from its `begin()` and `end()` function based on its constness, it stops being a `simple-view`, and thus it's not clear whether the change is due to `!simple-view<View>` or `!simple-view<Pattern>`. If `View` returns the same iterator types regardless of its constness, then I don't know of a way to distinguish between `__outer_iterator<false>` and `__outer_iterator<true>`.

I was tempted to make `_Const` a public member of `__outer_iterator` so that it can be checked in tests directly (but still is unaccessible to users). What do you think? Do you know if there's any precedent related to this?

================
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 {
+
----------------
philnik wrote:
> 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.
Unfortunately, it doesn't work -- without a name, it would be considered closing and reopening the same unnamed namespace, with the associated name clashes.

================
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
----------------
philnik wrote:
> 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.
Good catch, done.

================
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>>);
+
----------------
philnik wrote:
> Just for good measure, could you add `NoDefaultCtorForwardView, NoDefaultCtorForwardView`? Same for the `noexcept` test.
This came up in a previous review, and the outcome was to not add tests for `!A && !B`. The rationale was that, as long as `!A && B` and `A && !B` are tested, it is highly unlikely that `!A && !B` would behave differently, but adding these tests can result in a lot of boilerplate (not a big concern here, but I'd rather be consistent). So I'd rather keep as is.

================
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;
----------------
philnik wrote:
> Nit: I think the usual pattern is to have the function return `void` and assert inside the function. Same with `test_with_piping`.
I'll address this comment tomorrow.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107500