This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P2446R2 (views::as_rvalue)

Authored by philnik on Nov 8 2022, 6:05 AM.


Diff Detail

Event Timeline

philnik created this revision.Nov 8 2022, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 6:05 AM
philnik requested review of this revision.Nov 8 2022, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 6:05 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 474255.Nov 9 2022, 6:35 AM

Address Arthur's comments

philnik updated this revision to Diff 474259.Nov 9 2022, 6:43 AM

Remove non-ASCII character

philnik updated this revision to Diff 474521.Nov 10 2022, 5:17 AM

Add deduction guide

huixie90 added inline comments.Nov 10 2022, 6:43 AM
51 ↗(On Diff #474521)

might be worth testing that if underlying iterator is already a move_iterator, this is calling the move constructor of move_iterator instead of creating a move_iterator<move_iterator<It>>

ldionne requested changes to this revision.Dec 1 2022, 9:23 AM

@var-const Can you make a pass at this?

53 ↗(On Diff #474521)

Can you please add a simple test that exercises this ctad, even if it is the default-provided one by the compiler?

61 ↗(On Diff #474521)

Do you have tests for a simple view and a non-simple view?

74 ↗(On Diff #474521)

Do you have tests for a common range and a non-common range?

94 ↗(On Diff #474521)

Do you have a test that would fail if you dropped the all_t here?

103 ↗(On Diff #474521)

This [[nodiscard]] is an extension, so it should be using _LIBCPP_NODISCARD_EXT unless I am mistaken. Please make sure we have a libc++ specific test for this extension.

We might be doing it wrong for other adaptors, in which case we should also fix them, but not in this patch.


Should we be using the new facilities with meta::for_each here? (everywhere)

994 ↗(On Diff #474521)

Any reason why this is not constexpr when it can be? (everywhere)

This revision now requires changes to proceed.Dec 1 2022, 9:23 AM
var-const added inline comments.Dec 2 2022, 3:02 PM
3 ↗(On Diff #474521)

Please update the <ranges> synopsis as well.

30 ↗(On Diff #474521)

This needs an include (probably currently relying on the transitive include from all.h).

110–114 ↗(On Diff #474521)

I think this (an optimization, essentially) deserves a comment.


I think this check would be better in test_iterators.h, right after the definition of rvalue_iterator.


Can you also check piping a container (i.e. something that is not a view) through as_rvalue? Ideally, both lvalue and rvalue.


Nit: can you add empty lines between the cases? (Here and in some other tests)


s/auto/decltype(auto)/ (throughout).


The argument order is swapped in the comment.


Nit: return 0;.


These checks don't seem to be exhaustive. For example, there should probably be a check that !HasBegin if !range<const _View>.


This only tests the non-const version of begin, right? I think it shouldn't be too hard to also test a const as_rvalue_view here.


Optional: I think you could wrap this call in a check like if constexpr (std::sentinel_for<Iter, Iter>) and avoid having to call test_range directly from test.




Can you also test the case where the concept is false?


Is it possible to just use the std::ranges::common_range concept?


Same as the begin tests -- I think this should also test when a view is const.


Same as the begin test -- I think this can be simplified with if constexpr.


Why are we checking begin here?


Doesn't seem to be checked. Also, why is it only on the const sized view and not the non-const one?


Why is the return value only checked for the const case?

var-const requested changes to this revision.Dec 2 2022, 3:03 PM
huixie90 added inline comments.Dec 3 2022, 1:23 PM
34–36 ↗(On Diff #474521)

do we have a test for this?

38 ↗(On Diff #474521)

do we have a test for this? and a test for explicit?

philnik updated this revision to Diff 487170.Jan 8 2023, 5:25 AM
philnik marked 28 inline comments as done.

Address comments

110–114 ↗(On Diff #474521)

I'm not sure what you mean? This is required by the standard.


Yes, but I'd rather avoid the meta-programming and just pass where it's expected to be a common range.

var-const accepted this revision as: var-const.Jan 9 2023, 3:29 PM

LGTM (with just a couple of minor comments). I'll leave final approval to @ldionne since he had comments as well.

110–114 ↗(On Diff #474521)

We can still add a comment, even if this is essentially copied from the Standard. If I'm reading this correctly, this avoids wrapping a range in an rvalue_view if it's already a range of rvalues, to avoid unnecessary bloat. If that's correct, I think it deserves a comment.

38 ↗(On Diff #487170)

Would std::is_convertible work?

philnik updated this revision to Diff 489669.Jan 16 2023, 6:44 PM

Address comments

philnik marked an inline comment as done.Jan 16 2023, 6:46 PM
philnik added inline comments.
110–114 ↗(On Diff #474521)

AFAIK lots of views do this, and we don't comment anywhere else. Adding a comment here makes it look like this is an extension.

38 ↗(On Diff #487170)

Not for the default constructible case.

ldionne accepted this revision.Jan 19 2023, 8:38 AM
ldionne added inline comments.
53 ↗(On Diff #474521)

I don't think this was addressed. Please try to ensure that you've addressed feedback before marking a comment as done, otherwise it's really easy to drop (potentially important) comments on the floor. Reviewers usually assume that the feedback has been implemented correctly when the comment is marked as done.

110–114 ↗(On Diff #474521)

I would agree that no comment is needed since this is indeed what the standard specifies. I'm actually not sure what the comment would say.

This revision is now accepted and ready to land.Jan 19 2023, 8:38 AM
philnik updated this revision to Diff 490617.Jan 19 2023, 11:36 AM
philnik marked an inline comment as done.

Try to fix CI

philnik updated this revision to Diff 490641.Jan 19 2023, 1:07 PM
philnik marked 3 inline comments as done.
  • Try to fix CI
  • Address comments
This revision was landed with ongoing or failed builds.Jan 19 2023, 9:00 PM
This revision was automatically updated to reflect the committed changes.