This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] Implement P2494R2 (Relaxing range adaptors to allow for move only types)
ClosedPublic

Authored by yronglin on May 28 2023, 6:21 AM.

Details

Summary

Implement P2494R2 Relaxing range adaptors to allow for move only types

https://open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2494r2.html#wording-ftm

According to the words in P2494R2, I haven't add new test for drop_while_view, take_while_view and filter_view, because these views has the requirement that the predicate is an indirect_unary_predicate, which requires that the predicate is copy_constructible, so they still can't accept move only types as predicate.

[P2483R0] also suggests future work to relax the requirements on the predicate types stored by standard views. This paper does not perform this relaxation, as the copy constructibility requirement is enshrined in the indirect callable concepts ([indirectcallable.indirectinvocable]). Thus, while this paper modifies the views that currently use copyable-box for user provided predicates, it only does so to apply the rename of the exposition-only type to movable-box; it does not change any of the constraints on those views. It does, however, relax the requirements on invocables accepted by the transform family of views, because those are not constrained using the indirect callable concepts.

Diff Detail

Event Timeline

yronglin created this revision.May 28 2023, 6:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2023, 6:21 AM
Herald added a subscriber: arichardson. · View Herald Transcript
yronglin updated this revision to Diff 526329.May 28 2023, 7:50 AM

Try fix CI.

yronglin edited the summary of this revision. (Show Details)May 28 2023, 7:53 AM
yronglin updated this revision to Diff 526335.May 28 2023, 8:14 AM
yronglin edited the summary of this revision. (Show Details)

try fix ci

yronglin retitled this revision from [libcxx] Implement P2494R2 (Relaxing range adaptors to allow for move only types) to [libc++] Implement P2494R2 (Relaxing range adaptors to allow for move only types).May 30 2023, 6:51 AM
yronglin updated this revision to Diff 526598.May 30 2023, 6:58 AM

Remove extra format change.

yronglin published this revision for review.May 30 2023, 7:09 AM
yronglin added reviewers: philnik, ldionne, huixie90.
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2023, 7:09 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
yronglin retitled this revision from [libc++] Implement P2494R2 (Relaxing range adaptors to allow for move only types) to [libc++][ranges] Implement P2494R2 (Relaxing range adaptors to allow for move only types).May 30 2023, 11:07 AM
Mordante added a subscriber: Mordante.

I mainly glossed over it. I'm not to familiar with the details of these internals of ranges.

libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/properties.compile.pass.cpp
11 ↗(On Diff #526627)
yronglin updated this revision to Diff 527399.Jun 1 2023, 6:45 AM

Address comments

yronglin marked an inline comment as done.Jun 1 2023, 6:46 AM

Thanks for your review @Mordante !

var-const requested changes to this revision.Jun 2 2023, 5:16 PM

Thanks for the patch!

libcxx/include/__ranges/filter_view.h
56

For each of these changes to existing views, we need a new test(s) that would fail if not for the change in this patch.

libcxx/include/__ranges/movable_box.h
10 ↗(On Diff #527399)

The formatting changes make it harder to see things that are relevant to the proposal in this file. Can you clang-format copyable_box.h in a separate patch and rebase this patch on that?

33 ↗(On Diff #527399)

Is this comment up-to-date?

44 ↗(On Diff #527399)

Is there a reason not to apply the new behavior to old language versions retroactively? Would this change break any existing code?

(Also tagging @ldionne )

libcxx/include/__ranges/single_view.h
42

Can you please undo the formatting change? It makes it harder to see the parts of the diff that implement the proposal.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/arrow.pass.cpp
11

s/copyable-box/movable-box/. Applies throughout.

This revision now requires changes to proceed.Jun 2 2023, 5:16 PM

Do you need to also bump #define __cpp_lib_ranges ?

yronglin marked an inline comment as done.EditedJun 5 2023, 5:55 AM

Sorry for the late reply and thanks a lot for your review! @var-const

libcxx/include/__ranges/movable_box.h
10 ↗(On Diff #527399)

Ok, I'll rename copyable_box.h -> movable_box.h and formart in a separate patch.

33 ↗(On Diff #527399)

Oops, It need update.

libcxx/include/__ranges/single_view.h
42

OK, I'll undo format changes,

yronglin marked an inline comment as done.Jun 5 2023, 5:58 AM

Do you need to also bump #define __cpp_lib_ranges ?

Good catch! __cpp_lib_ranges need update to 202207L

ldionne added inline comments.
libcxx/include/__ranges/movable_box.h
44 ↗(On Diff #527399)

https://github.com/cplusplus/papers/issues/1156 is not super clear as to whether this should be treated as a DR.

@jwakely @CaseyCarter what did you folks do here? Did you apply P2494R2 (copyable-box => movable-box) as a DR to C++20, or do you consider it as a C++23 feature?

jwakely added inline comments.Jun 5 2023, 1:43 PM
libcxx/include/__ranges/movable_box.h
44 ↗(On Diff #527399)

https://github.com/cplusplus/papers/issues/1156 is not super clear as to whether this should be treated as a DR.

Good, I prefer to leave that to implementors where there's no compelling reason it has to be a DR. IMHO there's no defect here, this is a design change. It's an improvement, but the previous design was sound, not buggy.

@jwakely @CaseyCarter what did you folks do here? Did you apply P2494R2 (copyable-box => movable-box) as a DR to C++20, or do you consider it as a C++23 feature?

We didn't do it yet, but I think it looks harmless to apply the change to C++20 instead of making it conditional.

yronglin updated this revision to Diff 529625.Jun 8 2023, 9:18 AM

Undo format changes and add tests for move only types.

The header file was not renamed, and still __copyable_box.h, this makes the review more convenient, I will rename files and dirs after reviews done or in a separate patch.

Due to the words in P2494R2, I haven't add new test for drop_while_view, take_while_view and filter_view, because these views has the requirement that the predicate is an indirect_unary_predicate, which requires that the predicate is copy_constructible, so they still can't accept move only types as predicate.

[P2483R0] also suggests future work to relax the requirements on the predicate types stored by standard views. This paper does not perform this relaxation, as the copy constructibility requirement is enshrined in the indirect callable concepts ([indirectcallable.indirectinvocable]). Thus, while this paper modifies the views that currently use copyable-box for user provided predicates, it only does so to apply the rename of the exposition-only type to movable-box; it does not change any of the constraints on those views. It does, however, relax the requirements on invocables accepted by the transform family of views, because those are not constrained using the indirect callable concepts.
yronglin updated this revision to Diff 529636.Jun 8 2023, 9:33 AM

Fix feature test macro

yronglin edited the summary of this revision. (Show Details)Jun 8 2023, 9:34 AM

Do you need to also bump #define __cpp_lib_ranges ?

Thanks, done.

Normally the "Build Status" looks like this:

Buildable 237942
Build 368504: pre-merge checks
Build 368503: arc lint + arc unit

And here it is missing the last line. Is this some sort of CI issue?

Build Status
Buildable 237526
Build 367796: pre-merge checks

Normally the "Build Status" looks like this:

Buildable 237942
Build 368504: pre-merge checks
Build 368503: arc lint + arc unit

And here it is missing the last line. Is this some sort of CI issue?

Build Status
Buildable 237526
Build 367796: pre-merge checks

This looks weird, and I'm not sure what's going on, but it looks like the test results are looks good https://buildkite.com/llvm-project/diff-checks/builds/177081

CaseyCarter added inline comments.Jun 14 2023, 5:37 PM
libcxx/include/__ranges/movable_box.h
44 ↗(On Diff #527399)

We did _not_ apply to C++20 (https://github.com/microsoft/STL/pull/2965/files#diff-522242e575c221b356ad37137cf877b2a4b4a5de24333f834cf4f981560dd7ecR49-R56), but I could very easily be convinced that we should.

yronglin added inline comments.Jun 16 2023, 9:51 AM
libcxx/include/__ranges/movable_box.h
44 ↗(On Diff #527399)

Ahhh, thanks for your reply! Will you apply this change for c++20 in libstdc++ and microsoft/STL in the future?

ldionne added inline comments.Jun 16 2023, 1:29 PM
libcxx/include/__ranges/movable_box.h
44 ↗(On Diff #527399)

Looks like there's no strong reason to do it for C++20. I agree with @jwakely that this is not a "bug fix". So then I recommend we implement it as a new feature for C++23.

We can still rename the class to __movable_box while adding a comment that it actually requires copyability pre C++23.

yronglin updated this revision to Diff 532392.Jun 17 2023, 6:15 AM

Relaxing range adaptors to allow for move only types since C++23.

var-const requested changes to this revision.Jun 21 2023, 5:27 PM

Almost LGTM. Please also rename copyable_box.h to movable_box.h and I think this patch should be good to go.

libcxx/include/__ranges/copyable_box.h
41

Nit: consider rephrasing this so that it's written from the perspective of the new state, not the previous state. That is, instead of saying "we allow this since C++23", consider saying "this wasn't allowed before C++23". Perhaps something like:

Note: until C++23, `__movable_box` was named `__copyable_box` and required the stored type to be copy-constructible, not just move-constructible; we preserve the old behavior in pre-C++23 modes.
libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp
102

Nit: we usually rely on the compiler deducing the size of the array from the initializer.

This revision now requires changes to proceed.Jun 21 2023, 5:27 PM
yronglin updated this revision to Diff 533851.Jun 22 2023, 8:03 PM

Rename file name from copyable_box.h to movable_box.h

Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2023, 8:03 PM

Almost LGTM. Please also rename copyable_box.h to movable_box.h and I think this patch should be good to go.

Thanks for your review!

yronglin marked 2 inline comments as done.Jun 22 2023, 8:05 PM
yronglin updated this revision to Diff 533975.Jun 23 2023, 8:46 AM
yronglin edited the summary of this revision. (Show Details)

Try fix ci.

var-const accepted this revision.Jun 23 2023, 10:38 AM

Thank you for working on this!

This revision is now accepted and ready to land.Jun 23 2023, 10:38 AM
yronglin updated this revision to Diff 534153.Jun 23 2023, 9:28 PM

Thanks for your review! once the CI green, I'll land this patch.

yronglin updated this revision to Diff 534206.EditedJun 24 2023, 7:11 AM

NFC

  • Rename directory name from range.copy.warp to range.move.warp
  • Format

The windows CI failure seems not caused by this patch, I think it caused by D153656

yronglin edited the summary of this revision. (Show Details)Jun 24 2023, 5:10 PM
This revision was landed with ongoing or failed builds.Jun 25 2023, 8:27 AM
This revision was automatically updated to reflect the committed changes.