Page MenuHomePhabricator

[libc++][ranges] Add ranges::out_value_result
Needs RevisionPublic

Authored by philnik on Mar 10 2022, 6:41 PM.

Details

Reviewers
var-const
Quuxplusone
Mordante
huixie90
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

philnik created this revision.Mar 10 2022, 6:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 6:41 PM
Herald added a subscriber: mgorny. · View Herald Transcript
philnik requested review of this revision.Mar 10 2022, 6:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 6:41 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const requested changes to this revision.Mar 17 2022, 6:43 PM
var-const added inline comments.
libcxx/include/__algorithm/out_value_result.h
24

I think this is no longer necessary? Should it check for C++20 and later instead?

libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp
11

I think this is no longer necessary as well.

12

Hmm, but the header file isn't guarded on incomplete ranges?

27

Nit: a few of these lines are longer than 120 columns.

Optional: maybe use a type alias to cut down the boilerplate? I find it a little hard to separate the useful information in each check.

template <class T, class U>
using Res = std::ranges::out_value_result<T, U>; // Any short name would do
36

Question: would it make sense to also check rvalue references for good measure?

41

Nit: a similar check above uses a value, not a reference, for the second template argument. Can this be consistent? (assuming either can be used in this case)

64

A few test cases I'd like to add:

  • check that out_value_result is trivial and standard layout when it is true of the arguments;
  • check default construction;
  • check that out_value_result is copyable and movable if the arguments are (I think a static_assert is enough);
  • check that the rvalue overload of the conversion operator actually moves the arguments (by using a helper type that counts the number of copies and moves);
  • check that the [[no_unique_address]] optimization is used;
  • maybe check aggregate initialization (might be overkill).
65

Can you please add a comment to each test case that briefly explains what it does?

This revision now requires changes to proceed.Mar 17 2022, 6:43 PM
var-const added inline comments.Mar 17 2022, 7:09 PM
libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp
64

check that the [[no_unique_address]] optimization is used;

Ah, I see that it's addressed in D121435.

var-const added inline comments.Mar 17 2022, 7:17 PM
libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp
64

I spoke too soon -- while D121435 adds a few such tests, it doesn't address out_value_result, so the original comment still stands.

philnik updated this revision to Diff 420511.Apr 5 2022, 8:09 AM
philnik marked 9 inline comments as done.
  • Update to reflect changes from D121435

I haven't read all you comments @var-const. I updated the tests reflect the changes from D121435, that should address all your comments. If it doesn't, then we might be missing test coverage in all tests. I also updated the naming style here, which I will do for the other result types in a different PR.

var-const requested changes to this revision.Apr 7 2022, 4:56 PM

I haven't read all you comments @var-const. I updated the tests reflect the changes from D121435, that should address all your comments.

I think a few of those comments are still relevant (unless I'm missing something).

libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp
27

This is still relevant.

64

Still relevant.

65

Still relevant.

This revision now requires changes to proceed.Apr 7 2022, 4:56 PM
philnik updated this revision to Diff 452496.Aug 14 2022, 1:45 AM
philnik marked 3 inline comments as done.
  • Address comments
huixie90 accepted this revision as: huixie90.Aug 15 2022, 1:05 PM
huixie90 added a subscriber: huixie90.

LGTM with comments addressed and green CI

libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp
65

I would also add a runtime test to check out the && overload works (for move only types)

huixie90 accepted this revision.Aug 15 2022, 1:05 PM

LGTM with green CI and comments addressed

var-const requested changes to this revision.Aug 18 2022, 2:34 PM
var-const added inline comments.
libcxx/include/__algorithm/out_value_result.h
23

Please don't forget to remove this after rebasing (due to https://reviews.llvm.org/D132151).

libcxx/test/std/algorithms/algorithms.results/in_out_result.pass.cpp
94 ↗(On Diff #420511)

return 0 should stay, unless there was a recent change regarding this that I'm not aware of.

libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp
64

A few test cases I'd like to add:

  • check that out_value_result is trivial and standard layout when it is true of the arguments;
  • check default construction;
  • check that out_value_result is copyable and movable if the arguments are (I think a static_assert is enough);
  • check that the rvalue overload of the conversion operator actually moves the arguments (by using a helper type that counts the number of copies and moves);
  • check that the [[no_unique_address]] optimization is used;
  • maybe check aggregate initialization (might be overkill).

I think a few of these are still not addressed -- in particular, trivial check, default construction and that the arguments are actually moved.

This revision now requires changes to proceed.Aug 18 2022, 2:34 PM