[libcxx-commits] [PATCH] D144767: [libc++][ranges] Implement P2443R1: `views::chunk_by`
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Jun 1 22:45:29 PDT 2023
var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.
Thank you for working on this! Sorry I'm slow with the review.
================
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_;
----------------
Question: can `__base_` actually have be empty?
================
Comment at: libcxx/include/__ranges/chunk_by_view.h:58
+ _LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
+ _LIBCPP_NO_UNIQUE_ADDRESS __copyable_box<_Pred> __pred_;
+
----------------
Can you please add a TODO to update this to `__movable_box` once D151629 lands?
================
Comment at: libcxx/include/__ranges/chunk_by_view.h:62
+ using _Cache = __non_propagating_cache<iterator_t<_View>>;
+ _Cache __cached_begin_ = _Cache();
+
----------------
Is it necessary to explicitly call the default constructor of `__non_propagating_cache` here?
================
Comment at: libcxx/include/__ranges/chunk_by_view.h:68
+ _LIBCPP_ASSERT(
+ __pred_.__has_value(), "Trying to call __find_next() on a chunk_by_view that does not have a valid predicate.");
+ return ranges::next(ranges::adjacent_find(__current, ranges::end(__base_), std::not_fn(std::ref(*__pred_))),
----------------
Please add a blank line after this line.
================
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(
----------------
Nit: I'd `s/with begin iterator/on a begin iterator/`.
================
Comment at: libcxx/include/__ranges/chunk_by_view.h:79
+ _LIBCPP_ASSERT(
+ __pred_.__has_value(), "Trying to call __find_prev() on a chunk_by_view that does not have a valid predicate.");
+ auto __first = ranges::begin(__base_);
----------------
Please add a blank line after this line.
================
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_);
----------------
Nit: can you add a blank line after this line to separate error checking from the main flow of the function?
================
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:
> 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?
================
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:
> > 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?
================
Comment at: libcxx/include/__ranges/chunk_by_view.h:150
+ _LIBCPP_HIDE_FROM_ABI constexpr value_type operator*() const {
+ _LIBCPP_ASSERT(__current_ != __next_, "Trying to dereference past end chunk_by_view iterator.");
+ return {__current_, __next_};
----------------
Nit: `s/past end/a past-the-end/`.
================
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
----------------
Would any tests fail if we removed `decay_t` here?
================
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(
----------------
Mordante wrote:
> 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.
Hmm, I don't feel too strongly, but I actually wrote a comment asking to do the opposite before I read this thread. I think that ideally we'd assert in public functions (so that the name difference doesn't apply), but if the Standard wants us to assert in private functions, I'd go with the standard name (which is the name of the function we're implementing) rather than our internal name (for the same reason that I would use non-uglified template argument names, etc.). Do we have any precedent for this?
================
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{});
----------------
Question: is this the only (or easiest) way to get an invalid predicate? Would a default-constructed view work here, perhaps?
================
Comment at: libcxx/test/libcxx/transitive_includes/cxx11.csv:681
ranges limits
+ranges new
ranges optional
----------------
Question: why is this include added?
================
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;
----------------
Question: is it possible to simply compare for equality, or do we necessarily have to use `mismatch`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp:60
+ auto [chunk_e1, chunk_e2] = std::ranges::mismatch(*b1, *b2, [](int x, int y) {
+ const bool result = x == y;
+ assert(result);
----------------
Nit: we normally don't use "shallow" const much, and in this small scope in particular it seems unnecessary.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp:90
+
+ // Test `views::chunk_by(pred)(v)
+ {
----------------
Ultranit: unclosed backtick.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp:186
+ // That makes no sense and we can't do anything with the result, but it's valid.
+ { [[maybe_unused]] auto partial = std::views::chunk_by(3.14159); }
+
----------------
Nit: please add newlines (here and in other similar places):
```
{
[[maybe_unused]] auto partial = std::views::chunk_by(3.14159);
}
```
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/begin.pass.cpp:90
+
+ // begin() over a 8-element range
+ {
----------------
Ultranit: `s/a/an/`. Consider rephrasing the comment to something like `over a longer range`, though (the exact number isn't really important here, unlike in the previous two cases).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/begin.pass.cpp:105
+ std::ranges::chunk_by_view view(range, TrackingPred(&moved, &copied));
+ assert(std::exchange(moved, false));
+ [[maybe_unused]] auto it = view.begin();
----------------
Optional: I don't think we need to assert this since we're testing `begin`, not the constructor.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/begin.pass.cpp:146
+ assert(base((*it).end()) == buff + 4);
+ assert(called == 4);
+ }
----------------
Can you add a brief comment to explain why the number is `4`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/begin.pass.cpp:151
+constexpr bool test() {
+ general_tests();
+ cache_tests();
----------------
I would inline these functions. Normally we use named functions in tests when they are reused.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/constraints.compile.pass.cpp:31
+
+// chunk_by_view is not valid when the view is not an forward_range
+namespace test1 {
----------------
Ultranit: `s/an/a/`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/constraints.compile.pass.cpp:34
+struct View : std::ranges::view_base {
+ struct NotForwardIterator {
+ using value_type = int;
----------------
It looks like types from `almost_satisfies_types.h` would be helpful here.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/constraints.compile.pass.cpp:58
+// chunk_by_view is not valid when the predicate is not indirect_binary_predicate
+namespace test2 {
+struct View : std::ranges::view_base {
----------------
Nit: can these be more descriptive names?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/constraints.compile.pass.cpp:90
+// chunk_by_view is not valid when the predicate is not an object type
+namespace test4 {
+struct View : std::ranges::view_base {
----------------
Nit: can you add blank lines after opening and before closing a namespace?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/ctor.default.pass.cpp:75
+ }
+ assert(b1 == e1);
+ assert(b2 == e2);
----------------
Optional: I think it would be a little easier to compare the sizes of the inputs, it can simplify the loop as well if you could rely on them being the same size.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/ctor.default.pass.cpp:80
+constexpr bool test() {
+ {
+ using View = std::ranges::chunk_by_view<DefaultConstructibleView, DefaultConstructiblePredicate>;
----------------
Please add brief comments to each test case.
================
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();
----------------
Question: can a default-constructed `chunk_by_view` contain a valid predicate?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/compare.pass.cpp:92
+constexpr void test_with_pair() {
+ // Test with pair of iterators
+ test<Iterator>();
----------------
If I'm reading this correctly, the comments should be reversed -- `test<Iterator>` tests with a sentinel, and `test<Iterator, Iterator>` tests with a pair of iterators.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/compare.pass.cpp:100
+constexpr bool tests() {
+ test<forward_iterator<int*>>();
+ test<bidirectional_iterator<int*>>();
----------------
You can use `types::for_each` to reduce boilerplate here.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/ctor.default.pass.cpp:25
+
+enum class IsNoexcept : bool { no, yes };
+
----------------
Note: honestly, I think it's a little overkill (to be clear, I'm not requesting to change it), the scope of the test is very small and I think a simple boolean with a comment would have sufficed (it would also avoid having to call `to_underlying`).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/decrement.pass.cpp:72
+
+ // Test with a two chunks
+ {
----------------
Nit: `s/a two chunks/two chunks/`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/decrement.pass.cpp:139
+
+ // Test with predicate that is invocable but not callable
+ {
----------------
Ultranit: `s/predicate/a predicate/`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/deref.pass.cpp:37
+
+ for (ChunkByIterator iter = view.begin(); iter != view.end(); ++iter) {
+ std::same_as<typename ChunkByIterator::value_type> auto chunk = *iter;
----------------
Nit: if the intention is to check the return type, perhaps use `std::same_as` for the iterator?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/deref.pass.cpp:39
+ std::same_as<typename ChunkByIterator::value_type> auto chunk = *iter;
+ assert(std::ranges::distance(chunk) == 4);
+ }
----------------
Can we check the exact contents of each chunk instead?
================
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*>>();
----------------
This actually uses a sentinel, right? (and vice versa below)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/increment.pass.cpp:47
+
+ // Increment an iterator when it won't find another satisfied value after begin()
+ {
----------------
Ultranit: `s/an/the/`.
================
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
+ {
----------------
Question: can you elaborate?
================
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}; });
----------------
Nit: this formatting looks weird, is it produced by `clang-format`?
================
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*>>();
----------------
Using `types::for_each` would help reduce the boilerplate in this file (here and with the static assertions below too probably).
================
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>;
+
----------------
Question: why is this necessary? (Wouldn't the generated deduction guide do the same thing?)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144767/new/
https://reviews.llvm.org/D144767
More information about the libcxx-commits
mailing list