[libc++][ranges] Implement P2443R1: `views::chunk_by`

[JMazurkiewicz]

Summary

This patch implements https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2443r1.html (views::chunk_by).

[var-const]

libcxx/include/__ranges/chunk_by_view.h

78

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?

109

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?

109

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?

204

Would any tests fail if we removed decay_t here?

libcxx/test/libcxx/ranges/range.adaptors/range.chunk.by/assert.find-next.pass.cpp

27

Question: is this the only (or easiest) way to get an invalid predicate? Would a default-constructed view work here, perhaps?

libcxx/test/libcxx/transitive_includes/cxx11.csv

695

Question: why is this include added?

libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp

60

Question: is it possible to simply compare for equality, or do we necessarily have to use mismatch?

61

Nit: we normally don't use "shallow" const much, and in this small scope in particular it seems unnecessary.

91

Ultranit: unclosed backtick.

187

Nit: please add newlines (here and in other similar places):

{
  [[maybe_unused]] auto partial = std::views::chunk_by(3.14159);
}

libcxx/test/std/ranges/range.adaptors/range.chunk.by/begin.pass.cpp

91

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).

106

Optional: I don't think we need to assert this since we're testing begin, not the constructor.

147

Can you add a brief comment to explain why the number is 4?

152

I would inline these functions. Normally we use named functions in tests when they are reused.

libcxx/test/std/ranges/range.adaptors/range.chunk.by/constraints.compile.pass.cpp

32

Ultranit: s/an/a/.

35

It looks like types from almost_satisfies_types.h would be helpful here.

59

Nit: can these be more descriptive names?

91

Nit: can you add blank lines after opening and before closing a namespace?

libcxx/test/std/ranges/range.adaptors/range.chunk.by/ctor.default.pass.cpp

76

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.

81

Please add brief comments to each test case.

libcxx/test/std/ranges/range.adaptors/range.chunk.by/pred.pass.cpp

34

Question: can a default-constructed chunk_by_view contain a valid predicate?

libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/compare.pass.cpp

93

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.

101

You can use types::for_each to reduce boilerplate here.

libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/ctor.default.pass.cpp

26

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).

libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/decrement.pass.cpp

73

Nit: s/a two chunks/two chunks/.

140

Ultranit: s/predicate/a predicate/.

libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/deref.pass.cpp

38

Nit: if the intention is to check the return type, perhaps use std::same_as for the iterator?

40

Can we check the exact contents of each chunk instead?

45

This actually uses a sentinel, right? (and vice versa below)

libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/increment.pass.cpp

48

Ultranit: s/an/the/.

110

Question: can you elaborate?

113

Nit: this formatting looks weird, is it produced by clang-format?

libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/types.compile.pass.cpp

44

Using types::for_each would help reduce the boilerplate in this file (here and with the static assertions below too probably).

libcxx/test/std/ranges/range.adaptors/range.chunk.by/types.h

46

Question: why is this necessary? (Wouldn't the generated deduction guide do the same thing?)

[JMazurkiewicz]

• Rebase,
• Address almost all comments from @var-const (thank you for the review!),
• Enable std::ranges::chunk_by_view and std::views::chunk_by in std.cppm.

[JMazurkiewicz]

+comments

libcxx/include/__ranges/chunk_by_view.h

58

Yes, when it is for example ranges::empty_view.

63

It's not, I've removed it.

109

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?

109

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?

109

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.

204

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).

libcxx/test/libcxx/ranges/range.adaptors/range.chunk.by/assert.find-next.pass.cpp

27

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.

libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp

60

Yes, replaced with std::ranges::equal.

libcxx/test/std/ranges/range.adaptors/range.chunk.by/pred.pass.cpp

34

I don't think it is possible - default-constructed __copyable_box (or __movable_box) never contains a value.

libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/deref.pass.cpp

45

Yes, I've swapped comments again.

libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/increment.pass.cpp

110

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.

113

Yes, it is produced by clang-format. I'm going to fix it manually.

libcxx/test/std/ranges/range.adaptors/range.chunk.by/range.chunk.by.iter/types.compile.pass.cpp

44

Done for testValueTypeAndDifferenceType, but I don't think it is possible for static_assertions.

libcxx/test/std/ranges/range.adaptors/range.chunk.by/types.h

46

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.

[JMazurkiewicz]

Fix CI.

[JMazurkiewicz]

• Rebase,
• Fix CI: use __movable_box.

[JMazurkiewicz]

Rebase

[var-const]

Thanks for working on this! LGTM (with just one comment), and feel free to ping me if you run into any problems when rebasing.

libcxx/test/libcxx/ranges/range.adaptors/range.chunk.by/assert.begin.pass.cpp

14

Quick tip: when you rebase, you can replace D_LIBCPP_ENABLE_ASSERTIONS=1 with // UNSUPPORTED: !libcpp-has-hardened-mode && !libcpp-has-debug-mode.

libcxx/test/libcxx/ranges/range.adaptors/range.chunk.by/assert.find-next.pass.cpp

27

Thanks for explaining. I think it deserves a comment in the test file.

[Mordante]

@JMazurkiewicz friendly ping.

[JMazurkiewicz]

• Rebase,
• Target LLVM 18,
• Address more @var-const's comments.

[JMazurkiewicz]

Fix CI

[JMazurkiewicz]

Rebase and fix CI

[JMazurkiewicz]

Rebase, fix CI.

[var-const]

libcxx/include/__ranges/chunk_by_view.h

49

Quick question -- why do we need this?

[var-const]

libcxx/include/__ranges/chunk_by_view.h

49

Never mind -- it looks like it's to protect against macros like move.

[var-const]

@JMazurkiewicz Please let me know if you need any help to merge this patch.

Regarding the CI failure with reserved names -- I presume this is related to https://reviews.llvm.org/D147356, you might need to add the "push macros / pop macros" dance to ranges_adjacent_find.h and movable_box.h.

[JMazurkiewicz]

• Rebase
• Defend against move macro
• Replace _LIBCPP_ASSERT with _LIBCPP_ASSERT_UNCATEGORIZED
• Add _LIBCPP_PUSH_MACROS; #include <__undef_macros> ... _LIBCPP_POP_MACROS in ranges_adjacent_find.h and movable_box.h
• Try to fix CI and make this patch ready for the commit :)

[JMazurkiewicz]

Is this patch OK now? @var-const

If so, can you merge it for me? "Jakub Mazurkiewicz <[e-mail]>"

[var-const]

In D144767#4636804, @JMazurkiewicz wrote:

Is this patch OK now? @var-const

If so, can you merge it for me? "Jakub Mazurkiewicz <[e-mail]>"

Thank you for the patch! Merged now.

[ldionne]

This seems to break the C++20 CI: https://buildkite.com/llvm-project/libcxx-ci/builds/29475#018a6a62-4d93-4db9-bb60-0d011a40552e

Did you folks ensure a green build before merging? I'll try to take a look at what's wrong and fix it.

[JMazurkiewicz]

Did you folks ensure a green build before merging?

Yes, it was green.

Looks like views::chunk_by and ranges::chunk_by_view should be guarded with #if _LIBCPP_STD_VER >= 23 in modules/std/ranges.inc file. Do you want me to publish pull request with fix on Github? @ldionne

[ldionne]

Should be fixed by 98d28e947cc332f5bb9878e967ea5d7e414cf785! I reproduced and verified the fix locally.

I think what happened is a race condition with the enablement of C++23 modules in C++20. Basically the CI didn't run this test in C++20 mode when you triggered the CI. No big deal, it should work now.