Page MenuHomePhabricator

[libcxx][iterator] adds `indirectly_[regular_]unary_invocable` and `projected`
ClosedPublic

Authored by ldionne on Apr 25 2021, 10:17 PM.

Details

Summary

Also adds std::iter_common_reference_t and std::indirect_result_t.

Implements parts of:

  • P0896R4 The One Ranges Proposal

Diff Detail

Event Timeline

cjdb created this revision.Apr 25 2021, 10:17 PM
cjdb requested review of this revision.Apr 25 2021, 10:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2021, 10:17 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante added inline comments.Apr 26 2021, 8:46 AM
libcxx/include/__iterator/indirect_callable.h
31 ↗(On Diff #340434)

Please _Uglify and the rest of this file.

libcxx/test/std/iterators/iterator.requirements/indirectcallable/indirectcallable.indirectinvocable/indirect_result_t.compile.pass.cpp
26 ↗(On Diff #340434)

I would like to see some tests returning cv-qualified references.
Can you test some other invokable objects?

zoecarver added inline comments.Apr 26 2021, 9:09 AM
libcxx/test/std/iterators/iterator.requirements/indirectcallable/indirectcallable.indirectinvocable/indirect_result_t.compile.pass.cpp
26 ↗(On Diff #340434)

Can you test some other invokable objects?

There are some other interesting types in is_invocable.pass.cpp. Maybe one where we pick the correct overload, where there's a variadic (or even a template) involved, an explicit operator, a deleted overload, etc.

Would it make any sense to have a test where the result is void?

Also, maybe a few more negatives. Right now the only way we check that Is is indirectly_readable is that it's not int. That could be implemented typename std::iter_reference_t only. How about a type that is dereferenceable, but not iter_moveable.

ldionne commandeered this revision.Apr 28 2021, 10:32 AM
ldionne edited reviewers, added: cjdb; removed: ldionne.
ldionne updated this revision to Diff 348356.Thu, May 27, 12:33 PM
ldionne marked an inline comment as done.

Rebase, add tests, also implement equivalence_relation and strict_weak_order

ldionne added inline comments.Thu, May 27, 12:35 PM
libcxx/test/std/iterators/iterator.requirements/indirectcallable/indirectinvocable/indirect_strict_weak_order.compile.pass.cpp
2

This is basically copy-pasted from equivalence_relation because both are the same modulo semantic requirements.

libcxx/test/std/iterators/iterator.requirements/indirectcallable/indirectinvocable/indirectly_regular_unary_invocable.compile.pass.cpp
2

This is basically copy-pasted from indirect_unary_invocable because both are the same modulo semantic requirements.

LGTM. Thanks.

Only suggestion, does it make sense to add one or two tests where we are using this as a user would, instead of just checking the requirements are there? Maybe something like:

template<std::input_iterator I, std::sentinel_for<I> S,
         std::indirectly_unary_invocable<I> Fun>
constexpr auto myForEach(I first, S last, Fun f) const {
  for (; first != last; ++first) {
    std::invoke(f, *first);
  }
  return {first, f};
}

Or you could even test std::projected by projecting f.

I suppose these will come when we implement algs, such as for_each, so maybe it doesn't make sense.

libcxx/test/std/iterators/iterator.requirements/indirectcallable/indirectinvocable/indirectly_regular_unary_invocable.compile.pass.cpp
84

I won't object, but I feel that the volatile tests are a bit superfluous. I'm not sure they add much.

zoecarver accepted this revision as: zoecarver.Thu, May 27, 3:22 PM
cjdb accepted this revision.Thu, May 27, 3:32 PM
cjdb added inline comments.
libcxx/docs/OneRangesProposalStatus.csv
20–21 ↗(On Diff #348356)

I've got a big change coming for this csv, so I'd prefer to add it in mine (it'll screw up my doc right now).

libcxx/include/__iterator/indirect_concepts.h
95–104

This should probably get its own header.

ldionne marked 3 inline comments as done.Fri, May 28, 7:08 AM

LGTM. Thanks.

Only suggestion, does it make sense to add one or two tests where we are using this as a user would, instead of just checking the requirements are there? Maybe something like:

Yeah, I think it makes sense. I did it somewhat differently though, by adding asserts for a few less artificial predicates like [](int i) { return i % 2 == 0; } and simple things like that.

libcxx/test/std/iterators/iterator.requirements/indirectcallable/indirectinvocable/indirectly_regular_unary_invocable.compile.pass.cpp
84

Agreed - they were there in the original patch I commandeered, but I am OK removing them.

ldionne accepted this revision as: Restricted Project.Fri, May 28, 7:08 AM
ldionne marked an inline comment as done.
This revision is now accepted and ready to land.Fri, May 28, 7:08 AM
ldionne updated this revision to Diff 348522.Fri, May 28, 7:08 AM

Apply review comments.

This revision was landed with ongoing or failed builds.Fri, May 28, 7:09 AM
This revision was automatically updated to reflect the committed changes.