This patch implements https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2443r1.html (views::chunk_by).
Details
- Reviewers
var-const philnik ldionne huixie90 - Group Reviewers
Restricted Project - Commits
- rG065dc485bd4b: [libc++][ranges] Implement P2443R1: `views::chunk_by`
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?) |
- 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.
+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 |
| |
109 |
| |
109 |
I have no idea.
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. |
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. |
libcxx/include/__ranges/chunk_by_view.h | ||
---|---|---|
49 | Quick question -- why do we need this? |
libcxx/include/__ranges/chunk_by_view.h | ||
---|---|---|
49 | Never mind -- it looks like it's to protect against macros like move. |
@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.
- 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 :)
Is this patch OK now? @var-const
If so, can you merge it for me? "Jakub Mazurkiewicz <mazkuba3@gmail.com>"
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.
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
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.