[libcxx-commits] [PATCH] D144767: [libc++][ranges] Implement P2443R1: `views::chunk_by`

Sun Jun 25 08:47:57 PDT 2023

JMazurkiewicz added a comment.

+comments

================
Comment at: libcxx/include/__ranges/chunk_by_view.h:57
+class chunk_by_view : public view_interface<chunk_by_view<_View, _Pred>> {
+  _LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
+  _LIBCPP_NO_UNIQUE_ADDRESS __copyable_box<_Pred> __pred_;
----------------
var-const wrote:
> Question: can `__base_` actually have be empty?
Yes, when it is for example `ranges::empty_view`.

================
Comment at: libcxx/include/__ranges/chunk_by_view.h:62
+  using _Cache           = __non_propagating_cache<iterator_t<_View>>;
+  _Cache __cached_begin_ = _Cache();
+
----------------
var-const wrote:
> Is it necessary to explicitly call the default constructor of `__non_propagating_cache` here?
It's not, I've removed it.

================
Comment at: libcxx/include/__ranges/chunk_by_view.h:108
+    _LIBCPP_ASSERT(
+        __pred_.__has_value(), "Trying to call begin() on a chunk_by_view that does not have a valid predicate.");
+    auto __first = ranges::begin(__base_);
----------------
var-const wrote:
> var-const wrote:
> > var-const wrote:
> > > Nit: can you add a blank line after this line to separate error checking from the main flow of the function?
> > Hmm, it's interesting that the standard allows creating an unusable `chunk_by_view` -- just curious, do you know if there's a use case for that?
> I know it's in the standard, but it seems like this check is redundant with the one in `find-next`, or am I missing something?
> I know it's in the standard, but it seems like this check is redundant with the one in `find-next`, or am I missing something?

================
Comment at: libcxx/include/__ranges/chunk_by_view.h:108
+    _LIBCPP_ASSERT(
+        __pred_.__has_value(), "Trying to call begin() on a chunk_by_view that does not have a valid predicate.");
+    auto __first = ranges::begin(__base_);
----------------
JMazurkiewicz wrote:
> var-const wrote:
> > var-const wrote:
> > > var-const wrote:
> > > > Nit: can you add a blank line after this line to separate error checking from the main flow of the function?
> > > Hmm, it's interesting that the standard allows creating an unusable `chunk_by_view` -- just curious, do you know if there's a use case for that?
> > I know it's in the standard, but it seems like this check is redundant with the one in `find-next`, or am I missing something?
> > I know it's in the standard, but it seems like this check is redundant with the one in `find-next`, or am I missing something?
> 
> 
> I know it's in the standard, but it seems like this check is redundant with the one in `find-next`, or am I missing something?

================
Comment at: libcxx/include/__ranges/chunk_by_view.h:108
+    _LIBCPP_ASSERT(
+        __pred_.__has_value(), "Trying to call begin() on a chunk_by_view that does not have a valid predicate.");
+    auto __first = ranges::begin(__base_);
----------------
JMazurkiewicz wrote:
> JMazurkiewicz wrote:
> > var-const wrote:
> > > var-const wrote:
> > > > var-const wrote:
> > > > > Nit: can you add a blank line after this line to separate error checking from the main flow of the function?
> > > > Hmm, it's interesting that the standard allows creating an unusable `chunk_by_view` -- just curious, do you know if there's a use case for that?
> > > I know it's in the standard, but it seems like this check is redundant with the one in `find-next`, or am I missing something?
> > > I know it's in the standard, but it seems like this check is redundant with the one in `find-next`, or am I missing something?
> > 
> > 
> > I know it's in the standard, but it seems like this check is redundant with the one in `find-next`, or am I missing something?
> 
> 
> Hmm, it's interesting that the standard allows creating an unusable chunk_by_view -- just curious, do you know if there's a use case for that?

I have no idea.

> I know it's in the standard, but it seems like this check is redundant with the one in `find-next`, or am I missing something?

This check is not redundant. I've explained why in comment in `assert.find-next.pass.cpp` file.

================
Comment at: libcxx/include/__ranges/chunk_by_view.h:203
+  template <class _Pred>
+    requires constructible_from<decay_t<_Pred>, _Pred>
+  _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Pred&& __pred) const
----------------
var-const wrote:
> Would any tests fail if we removed `decay_t` here?
No... until now. I've added extra test coverage in `adaptor.pass.cpp` (marked with `Test that one can call std::views::chunk_by with arbitrary stuff (...)` comment).

================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.chunk.by/assert.find-next.pass.cpp:26
+  int input[] = {1, 2, 3};
+  auto view1  = std::views::chunk_by(input, ThrowOnCopyPred{});
+  auto view2  = std::views::chunk_by(input, ThrowOnCopyPred{});
----------------
var-const wrote:
> Question: is this the only (or easiest) way to get an invalid predicate? Would a default-constructed view work here, perhaps?
This is the easiest way (I think) to get `__find_next` to fail. If we used default constructed view here, then `begin()` would fail instead of `__find_next`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp:59
+  for (; b1 != e1 && b2 != e2; ++b1, ++b2) {
+    auto [chunk_e1, chunk_e2] = std::ranges::mismatch(*b1, *b2, [](int x, int y) {
+      const bool result = x == y;
----------------
var-const wrote:
> Question: is it possible to simply compare for equality, or do we necessarily have to use `mismatch`?
Yes, replaced with `std::ranges::equal`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/pred.pass.cpp:33
+    Pred pred{42};
+    std::ranges::chunk_by_view<Range, Pred> const view(Range{}, pred);
+    std::same_as<Pred const&> decltype(auto) result = view.pred();
----------------
var-const wrote:
> Question: can a default-constructed `chunk_by_view` contain a valid predicate?
I don't think it is possible - default-constructed `__copyable_box` (or `__movable_box`) never contains a value.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/deref.pass.cpp:44
+constexpr bool tests() {
+  // Check iterator pair
+  test<forward_iterator<int*>>();
----------------
var-const wrote:
> This actually uses a sentinel, right? (and vice versa below)
Yes, I've swapped comments again.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/increment.pass.cpp:109
+
+  // Test with predicate that is invocable but not callable
+  {
----------------
var-const wrote:
> Question: can you elaborate?
All callables can be called like regular function `f()`, but not all invocables can (we need to use `std::invoke` for them). This tests checks if we use `std::invoke` properly or we just use `pred()` syntax.

I've added comment explaining this here and in `decrement.pass.cpp`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/increment.pass.cpp:112
+    std::array array = {1, 2, 3, -3, -2, -1};
+    auto v           = View{Iterator{array.begin()}, Sentinel{Iterator{array.end()}}} |
+             std::views::transform([](int x) { return IntWrapper{x}; });
----------------
var-const wrote:
> Nit: this formatting looks weird, is it produced by `clang-format`?
Yes, it is produced by `clang-format`. I'm going to fix it manually.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/types.compile.pass.cpp:43
+  {
+    testValueTypeAndDifferenceType<forward_iterator<int*>>();
+    testValueTypeAndDifferenceType<bidirectional_iterator<int*>>();
----------------
var-const wrote:
> Using `types::for_each` would help reduce the boilerplate in this file (here and with the static assertions below too probably).
Done for `testValueTypeAndDifferenceType`, but I don't think it is possible for `static_assert`ions.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/types.h:45
+template <class I, class S>
+View(I b, S e) -> View<I, S>;
+
----------------
var-const wrote:
> Question: why is this necessary? (Wouldn't the generated deduction guide do the same thing?)
Yes, it is necessary. Without this deduction guide I get `error: 'View' may not intend to support class template argument deduction` warning, but since tests use `-Werror` this ends up as a hard error.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144767