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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

Thanks a lot! This LGTM with my comments applied and CI passing.



================
Comment at: libcxx/include/__ranges/lazy_split_view.h:3
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
You should rebase on top of 9a44ed43cf9a8d4e3e55d007432e363e094ec6b0. Basically, ` rm -rf libcxx/test/libcxx/diagnostics/detail.headers` and then regenerate auto-generated files.







================
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 &&>);
----------------
var-const wrote:
> 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.
I've actually had the same discussion with @philnik and he told me he was basically testing the test, which I had not realized at first. I think this is great. However, I think it should be sufficient to have just a single positive case to confirm that `CanCallBase` works as intended, and then the other ones can be runtime tests instead.

However, here it looks like the `const&&` and the `const&` variants don't have runtime tests below -- so we should add some and then remove the positive `static_assert`s corresponding to them above.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/assert.equal.pass.cpp:9
+
+// UNSUPPORTED: c++03, windows, libcxx-no-debug-mode
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DEBUG=0
----------------
This should also be unsupported in C++11, C++14, C++17, because we are using ranges.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/assert.equal.pass.cpp:10
+// UNSUPPORTED: c++03, windows, libcxx-no-debug-mode
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DEBUG=0
+
----------------
This should be `-D_LIBCPP_ENABLE_ASSERTIONS=1`. Please apply to other assertion tests too!


================
Comment at: libcxx/test/support/is_explicit_ctr.h:15
+
+// A pseudofunction that takes a `T` parameter to check whether passing `U` such that `U` is implicitly convertible to
+// `T` works.
----------------
Would it be sufficient to use `!std::is_convertible<T, U> && std::is_constructible<T, U>`? At that point, you might not need this helper at all, since you can probably just query `!std::is_convertible<T, U>` in your tests (constructibility is already tested by the fact that you construct objects in your tests).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107500