This patch implements https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2443r1.html (views::chunk_by).
Diff Detail
Event Timeline
Improve test coverage.
I haven't fixed the problem with transitive includes yet - does anyone know what might be the reason of this failure? (I've marked problematic includes with FIXME transitive include comment)
I think you should add ranges new beetween these two lines: https://github.com/llvm/llvm-project/blob/fac4c476b97b9f2a9dace059f084696d856c033b/libcxx/test/libcxx/transitive_includes/cxx03.csv#L647-L648 and https://github.com/llvm/llvm-project/blob/main/libcxx/test/libcxx/transitive_includes/cxx11.csv#L648-L649
In theory these files should be updated when you run ninja -C build libcxx-generate-files but that command doesn't work on Windows...
Ok, I think current CI failures are not your fault.
Look at the Scheduled build: https://buildkite.com/llvm-project/libcxx-ci/builds/20489
It also failed.
I think I fixed the CI when I revert the commit: https://github.com/llvm/llvm-project/commit/24416d22bb284cf24335ee7aa63131e5de56a1bb
And I restarted CI, so I hope it should pass now.
Thanks for working on this!
I haven't reviewed the tests yet, but I wanted to give you some feedback.
libcxx/include/__ranges/chunk_by_view.h | ||
---|---|---|
31 | Can you explain what issue you have with these transitive includes? | |
78 | 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. | |
81 | Now the object can be moved on line 85. | |
86 | Is there a reason why this is written in a different fashion as in the Standard? This makes it harder to verify its correctness. | |
110 | Without const this can be moved. Note that the libc++ code uses const very sparingly. | |
132 | Is there a reason to make the exposition only type public? | |
157 | Since you used _LIBCPP_ASSERT for most preconditions, it would be great to do the same here. | |
libcxx/include/ranges | ||
315 | Please align all since comments | |
libcxx/test/libcxx/ranges/range.adaptors/range.chunk.by/assert.begin.pass.cpp | ||
14–18 | Typically we stat the file with this part and then the header under test. | |
44 | Can you also test the other asserts you added? | |
libcxx/test/libcxx/transitive_includes/cxx11.csv | ||
685 | Did you remove headers or not? | |
libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp | ||
11 | Please add the header name here too. |
- Address some of @Mordante's comments
- Rename iterator directory to range.chunk.by.iter
TODO: transitive includes, verify more assertions
libcxx/include/__ranges/chunk_by_view.h | ||
---|---|---|
86 |
This is based on reference implementation from P2443R1 (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2443r1.html#pnum_16); I've only replaced not_fn(bind(ref(*pred_), _2, _1)) with lambda expression. | |
132 | No, I think that I wanted to test something here. |