Page MenuHomePhabricator

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

Authored by JMazurkiewicz on Feb 24 2023, 4:31 PM.

Details

Reviewers
var-const
philnik
ldionne
huixie90
Group Reviewers
Restricted Project
Summary

Diff Detail

Event Timeline

JMazurkiewicz created this revision.Feb 24 2023, 4:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2023, 4:31 PM
Herald added a subscriber: arichardson. · View Herald Transcript
JMazurkiewicz requested review of this revision.Feb 24 2023, 4:31 PM
JMazurkiewicz set the repository for this revision to rG LLVM Github Monorepo.Feb 24 2023, 5:09 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 24 2023, 5:09 PM

Try to fix CI.

JMazurkiewicz set the repository for this revision to rG LLVM Github Monorepo.

Fix CI (add missing includes), and update release notes.

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)

Fix formatting.

fsb4000 added a subscriber: fsb4000.EditedMar 3 2023, 11:09 PM

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

https://github.com/llvm/llvm-project/issues/57458

Update transitive includes manually.

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.

The tests passed and I added more reviewers to this patch.

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.

JMazurkiewicz marked 8 inline comments as done.
  • Address some of @Mordante's comments
  • Rename iterator directory to range.chunk.by.iter

TODO: transitive includes, verify more assertions

JMazurkiewicz added inline comments.Apr 18 2023, 5:35 AM
libcxx/include/__ranges/chunk_by_view.h
86

Is there a reason why this is written in a different fashion as in the Standard?

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.

  • Test all added assertions,
  • Rebase,
  • Fix CI?

TODO: transitive includes

JMazurkiewicz marked an inline comment as done.Apr 25 2023, 7:42 AM
JMazurkiewicz set the repository for this revision to rG LLVM Github Monorepo.Apr 25 2023, 12:07 PM

Rebase; try to fix CI.

Rebase; fix CI.