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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this!
I haven't reviewed the tests yet, but I wanted to give you some feedback.



================
Comment at: libcxx/include/__ranges/chunk_by_view.h:77
+  {
+    _LIBCPP_ASSERT(__current != ranges::begin(__base_), "Trying to call find-prev() with begin iterator.");
+    _LIBCPP_ASSERT(
----------------
please search for similar in the asserts and fix them too. Note users of libc++ tend not to read the Standard, so it's better to use the name used in libc++ than the name in the Standard.


================
Comment at: libcxx/include/__ranges/chunk_by_view.h:80
+        __pred_.__has_value(), "Trying to call find-prev() on a chunk_by_view that does not have a valid predicate.");
+    const auto __first = ranges::begin(__base_);
+    reverse_view __reversed{subrange{__first, __current}};
----------------
Now the object can be moved on line 85.


================
Comment at: libcxx/include/__ranges/chunk_by_view.h:85
+    };
+    return ranges::prev(ranges::adjacent_find(__reversed, __reversed_pred).base(), 1, __first);
+  }
----------------
Is there a reason why this is written in a different fashion as in the Standard? This makes it harder to verify its correctness.


================
Comment at: libcxx/include/__ranges/chunk_by_view.h:109
+        __pred_.__has_value(), "Trying to call begin() on a chunk_by_view that does not have a valid predicate.");
+    const auto __first = ranges::begin(__base_);
+    if (!__cached_begin_.__has_value()) {
----------------
Without `const` this can be moved. Note that the libc++ code uses `const` very sparingly.


================
Comment at: libcxx/include/__ranges/chunk_by_view.h:131
+class chunk_by_view<_View, _Pred>::__iterator {
+public:
+  friend chunk_by_view;
----------------
Is there a reason to make the exposition only type public?


================
Comment at: libcxx/include/__ranges/chunk_by_view.h:156
+  _LIBCPP_HIDE_FROM_ABI constexpr __iterator& operator++() {
+    __current_ = __next_;
+    __next_    = __parent_->__find_next(__current_);
----------------
Since you used `_LIBCPP_ASSERT` for most preconditions, it would be great to do the same here.


================
Comment at: libcxx/include/__ranges/chunk_by_view.h:30
+#include <__ranges/concepts.h>
+#include <__ranges/copyable_box.h>          // FIXME transitive include
+#include <__ranges/non_propagating_cache.h> // FIXME transitive include
----------------
Can you explain what issue you have with these transitive includes?


================
Comment at: libcxx/include/ranges:315
+    requires view<V> && is_object_v<Pred>
+  class chunk_by_view; // since C++23
+
----------------
Please align all since comments


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.chunk.by/assert.begin.pass.cpp:13-17
+// REQUIRES: has-unix-headers
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// UNSUPPORTED: no-exceptions
+// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx{{10.9|10.10|10.11|10.12|10.13|10.14|10.15|11.0|12.0}}
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_ASSERTIONS=1
----------------
Typically we stat the file with this part and then the header under test.


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.chunk.by/assert.begin.pass.cpp:43
+  TEST_LIBCPP_ASSERT_FAILURE(
+      view1.begin(), "Trying to call begin() on a chunk_by_view that does not have a valid predicate.");
+  return 0;
----------------
Can you also test the other asserts you added?


================
Comment at: libcxx/test/libcxx/transitive_includes/cxx11.csv:670
 ranges type_traits
-ranges variant
 ranges version
----------------
Did you remove headers or not?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp:10
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+// std::views::chunk_by
----------------
Please add the header name here too. 


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

https://reviews.llvm.org/D144767