Page MenuHomePhabricator

[libcxx][ranges] Add `indirectly_­movable` and `indirectly_movable_storable`.
ClosedPublic

Authored by zoecarver on May 17 2021, 10:30 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

.pass.cpp -> .comple.pass.cpp

cjdb requested changes to this revision.May 17 2021, 11:28 AM

Please add conformance and subsumption tests. It also makes sense to do both indirectly_movable and indirectly_movable_storable in the same patch, although I won't block on this.

libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp
34 ↗(On Diff #345933)

IndirectlyReadableMoveOnly is a more descriptive name. It's not really wrapping anything.

40–41 ↗(On Diff #345933)

Please add tests for lvalue arrays too.

This revision now requires changes to proceed.May 17 2021, 11:28 AM
Mordante accepted this revision as: Mordante.May 17 2021, 11:31 AM
Mordante added a subscriber: Mordante.

LGTM, except a minor issues. Please update the synopsis in <iterator>.

libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp
13 ↗(On Diff #345933)

template<class In, class Out>

Quuxplusone requested changes to this revision.May 17 2021, 11:49 AM
Quuxplusone added inline comments.
libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp
21 ↗(On Diff #345933)

I think we need some tests using a type with

const int& operator*() const;

or at least some tests using const int*. For example, std::indirectly_movable<const int*, int*> should be false (but std::indirectly_movable<int*, const int*> should be true). And std::indirectly_movable<MoveOnly*, const MoveOnly*> should also be false because MoveOnly is move-only.

  • Add indirectly_movable_storable to this patch.
  • Fix synopsis.
  • Add test for lvalue array types.

Please add conformance and subsumption tests.

Is it just the smart pointers which need these conformance tests, or others too?

cjdb added a comment.May 17 2021, 12:16 PM

Please add conformance and subsumption tests.

Is it just the smart pointers which need these conformance tests, or others too?

All of them do.

  • Update synopsis.
  • Add tests as suggested by Arthur.
  • Fix test synposis (again).
zoecarver updated this revision to Diff 345992.May 17 2021, 2:03 PM

Add conformance tests.

cjdb added inline comments.May 17 2021, 2:31 PM
libcxx/test/std/iterators/predef.iterators/insert.iterators/back.insert.iterator/iterator_concept_conformance.compile.pass.cpp
27–28

This should be applied in other output-only iterators too.

zoecarver updated this revision to Diff 345997.May 17 2021, 2:42 PM

Add remaining iterator concept tests.

cjdb requested changes to this revision.May 17 2021, 4:29 PM

Please don't forget the subsumption tests.

libcxx/test/std/containers/associative/map/iterator_concept_conformance.compile.pass.cpp
36–43

If an iterator isn't indirectly_writable, then it doesn't need to be on the RHS of this test.

libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp
94–104 ↗(On Diff #345997)

I think you're missing a test for when exactly one of each concept is false. Also, can we get these moved into an indirectly_movable_storable.cpp test please?

This revision now requires changes to proceed.May 17 2021, 4:29 PM
cjdb added a comment.May 18 2021, 10:26 AM

Also, please add indirectly_movable_storable to the commit subject.

zoecarver retitled this revision from [libcxx][ranges] Add `indirectly_­movable`. to [libcxx][ranges] Add `indirectly_­movable` and `indirectly_movable_storable`..May 20 2021, 1:24 PM
zoecarver added inline comments.May 20 2021, 2:56 PM
libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp
94–104 ↗(On Diff #345997)

Can you point out the cases that aren't covered? I think I've got them all, except maybe !constructible_from.

I think this is still a fairly small test file, and it's nice to be able to reuse types.

cjdb added inline comments.May 20 2021, 3:02 PM
libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp
94–104 ↗(On Diff #345997)

I don't see cases where only:

  • indirectly_movable is false
  • indirectly_writable is false
  • constructible_from is false
  • assignable_from is false
zoecarver updated this revision to Diff 346873.May 20 2021, 3:33 PM

Update based on review.

indirectly_movable is false

OK, I thought it would be OK not to add these, but I'll add a few.

indirectly_writable is false

Depending on exactly what you mean, MoveOnlyConvertibleWrapper is this case.

constructible_from is false
assignable_from is false

Added.

cjdb added a comment.May 20 2021, 4:56 PM

indirectly_writable is false

Depending on exactly what you mean, MoveOnlyConvertibleWrapper is this case.

I have no idea what MoveOnlyConvertibleWrapper is supposed to represent. Can you please give all your wrapper types more descriptive names?

ldionne requested changes to this revision.Jun 9 2021, 1:58 PM

Can you mark review comments that have been addressed as Done so it's easy to follow what's left? I got the feeling that you addressed some that are not marked as Done yet.

libcxx/include/__iterator/concepts.h
171

Do we want to move this to indirect_concepts.h? Or should we instead get rid of indirect_concepts.h and move those here instead (as a separate NFC patch)?

No strong preference.

libcxx/test/std/containers/associative/map/iterator_concept_conformance.compile.pass.cpp
37

Is this because iterator::value_type is std::pair<int const, int> and not std::pair<int, int>?

libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp
1 ↗(On Diff #346873)

I would like us to test one concept per file, like we do everywhere else.

This revision now requires changes to proceed.Jun 9 2021, 1:58 PM
ldionne added inline comments.Jun 10 2021, 10:26 AM
libcxx/test/std/containers/associative/map/iterator_concept_conformance.compile.pass.cpp
37

Per offline discussion, I think this should be using value_type* instead of pair<int, int>*.

zoecarver marked 8 inline comments as done.Jun 14 2021, 6:38 AM

@ldionne applying your comments now...


I have no idea what MoveOnlyConvertibleWrapper is supposed to represent. Can you please give all your wrapper types more descriptive names?

What would you suggest I name something like MoveOnlyConvertibleWrapper? Does ConvertibleToMoveOnlyWrapper work? Or is it the Wrapper part specifically that you don't like?

This is testing the case where we can convert to a type that is assignable to MoveOnly but it itself is not assignable to MoveOnly.

libcxx/test/std/containers/associative/map/iterator_concept_conformance.compile.pass.cpp
37

The reason this doesn't use value_type is because it's not assignable-to. In this case value_type is pair<const int, int> which we can't assign pair<const int, int> to. But we can assign pair<const int, int> to pair<int, int> so, we use that instead (because that's the only use-case where this would succeed).

Quuxplusone added inline comments.Jun 14 2021, 7:04 AM
libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp
72–76 ↗(On Diff #346873)

I think I agree with Chris's confusion here. Is this perhaps backwards? Did you mean to write

using value_type = MoveOnly;
MoveOnlyConvertible operator*() const;

so that this would be a lot like vector<bool> (i.e. its operator* returns a proxy convertible to value_type)? If so, I think VectorBool might be a decent name for it.

The problem with stringing particles together, like MoveOnly+Convertible+Wrapper, is that it's never clear what particle refers to what. Is this a wrapper around a MoveOnlyConvertible? Is it a wrapper that's convertible to a MoveOnly? Is it a move-only wrapper that is also convertible? Is it move-only wrapper around a Convertible? etc. It would be strictly less confusing to just name it struct A.

Another trick that can help with readability is to use nested types to keep related entities tightly coupled. If the only defining characteristic of MoveOnlyConvertible is that you can convert from it to MoveOnly, then it should probably be named MoveOnly::From.

zoecarver updated this revision to Diff 351870.Jun 14 2021, 7:37 AM
zoecarver marked 4 inline comments as done.
  • Move tests into their own files.
  • Rebase.
zoecarver added inline comments.Jun 14 2021, 7:45 AM
libcxx/include/__iterator/concepts.h
171

The latter option was selected. Thanks for doing this.

Fix two conformance tests.

zoecarver updated this revision to Diff 351977.Jun 14 2021, 1:11 PM

Fix merge conflict.

Rebase.

ldionne added inline comments.Jun 14 2021, 1:20 PM
libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable.compile.pass.cpp
27

This seems unused in this file.

37

And so does this function.

41–44

Suggestion:

template <class T>
struct PointerTo {
  using value_type = T;
  T& operator*() const;
};

Then use PointerTo<MoveOnly> instead of MoveOnlyWrapper in the tests below. I find it easier to understand that way, WDYT?

libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp
143

Should this be testing indirectly_movable or indirectly_movable_storable?

145

Same.

zoecarver updated this revision to Diff 351997.Jun 14 2021, 2:27 PM
zoecarver marked 5 inline comments as done.

Apply Louis' suggestions.

I'll look into Arthur's suggestion tomororw.

Thanks both for the review comments :)

ldionne requested changes to this revision.Jun 14 2021, 3:00 PM
ldionne added inline comments.
libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp
136

Was this one left behind on purpose?

This revision now requires changes to proceed.Jun 14 2021, 3:00 PM

@ldionne sorry, I just realized that I had some comments responding to various suggestions you made that never got submitted. Anyway, that's why I marked a few things as done but didn't actually do them.

libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable.compile.pass.cpp
41–44

Yes, that is more readable. Thanks :)

libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp
136

(Just realized I had an un submitted reply to your previous comment about this. Moving to this thread.)

Yes, I'm trying to demonstrate that indirectly_­writable<Out, iter_rvalue_reference_t<In>> works but indirectly_­writable<Out, iter_value_t<In>> does not.

I suppose maybe it would be better to just do static_assert(indirectly_­writable<PointerToWithValueType<MoveOnly, MoveOnlyConvertible>, iter_rvalue_reference_t<PointerToWithValueType<MoveOnly, MoveOnlyConvertible>>>) instead.

zoecarver updated this revision to Diff 353400.Jun 21 2021, 9:41 AM

Change static assert and add a comment.

ldionne accepted this revision.Jun 21 2021, 10:00 AM

LGTM once CI passes, and if you merge PointerToWithValueType and PointerTo as we discussed offline just now.

libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable.compile.pass.cpp
60

I think you mean PointerTo<MoveOnly> instead of PointerTo<MoveOnlyWrapper> here (and elsewhere).

ldionne added inline comments.Jun 21 2021, 10:01 AM
libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp
43–45

Also, this could be ConvertibleTo<MoveOnly> instead, and same for AssignableTo below.

Remove PointerToWithValueType and update PointerTo.

Rebase. Add some more comments.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 21 2021, 12:40 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.