Details
- Reviewers
cjdb • Quuxplusone EricWF ldionne Mordante - Group Reviewers
Restricted Project - Commits
- rG075f2370c7fa: [libcxx][ranges] Add `indirectly_movable` and `indirectly_movable_storable`.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
---|---|---|
35 | IndirectlyReadableMoveOnly is a more descriptive name. It's not really wrapping anything. | |
41–42 | Please add tests for lvalue arrays too. |
LGTM, except a minor issues. Please update the synopsis in <iterator>.
libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp | ||
---|---|---|
14 | template<class In, class Out> |
libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp | ||
---|---|---|
22 | 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?
libcxx/test/std/iterators/predef.iterators/insert.iterators/back.insert.iterator/iterator_concept_conformance.compile.pass.cpp | ||
---|---|---|
27–28 ↗ | (On Diff #345992) | This should be applied in other output-only iterators too. |
Please don't forget the subsumption tests.
libcxx/test/std/containers/associative/map/iterator_concept_conformance.compile.pass.cpp | ||
---|---|---|
36–43 ↗ | (On Diff #345997) | 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 | ||
95–105 | 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? |
libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp | ||
---|---|---|
95–105 | 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. |
libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp | ||
---|---|---|
95–105 | I don't see cases where only:
|
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.
I have no idea what MoveOnlyConvertibleWrapper is supposed to represent. Can you please give all your wrapper types more descriptive names?
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 | ||
---|---|---|
168 | 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 ↗ | (On Diff #346873) | 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 | ||
2 | I would like us to test one concept per file, like we do everywhere else. |
libcxx/test/std/containers/associative/map/iterator_concept_conformance.compile.pass.cpp | ||
---|---|---|
37 ↗ | (On Diff #346873) | Per offline discussion, I think this should be using value_type* instead of pair<int, int>*. |
@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 ↗ | (On Diff #346873) | 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). |
libcxx/test/std/iterators/iterator.requirements/alg.req/ind.move.compile.pass.cpp | ||
---|---|---|
73–77 | 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. |
libcxx/include/__iterator/concepts.h | ||
---|---|---|
168 | The latter option was selected. Thanks for doing this. |
libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable.compile.pass.cpp | ||
---|---|---|
26 ↗ | (On Diff #351870) | This seems unused in this file. |
36 ↗ | (On Diff #351870) | And so does this function. |
40–43 ↗ | (On Diff #351870) | 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 | ||
142 ↗ | (On Diff #351870) | Should this be testing indirectly_movable or indirectly_movable_storable? |
144 ↗ | (On Diff #351870) | Same. |
Apply Louis' suggestions.
I'll look into Arthur's suggestion tomororw.
Thanks both for the review comments :)
libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp | ||
---|---|---|
135 ↗ | (On Diff #351997) | Was this one left behind on purpose? |
@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 | ||
---|---|---|
40–43 ↗ | (On Diff #351870) | Yes, that is more readable. Thanks :) |
libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp | ||
135 ↗ | (On Diff #351997) | (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. |
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 | ||
---|---|---|
59 ↗ | (On Diff #351997) | I think you mean PointerTo<MoveOnly> instead of PointerTo<MoveOnlyWrapper> here (and elsewhere). |
libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp | ||
---|---|---|
42–44 ↗ | (On Diff #353400) | Also, this could be ConvertibleTo<MoveOnly> instead, and same for AssignableTo below. |
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.