Page MenuHomePhabricator

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

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

Diff Detail

Unit TestsFailed

300 mslibcxx CI C++20 > libcxx-trunk-shared.std/iterators/predef_iterators/move_iterators/move_iterator::iterator_concept_conformance.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/1ed10f485dfe-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/predef.iterators/move.iterators/move.iterator/iterator_concept_conformance.compile.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/1ed10f485dfe-1/llvm-project/libcxx-ci/build/generic-cxx20/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/1ed10f485dfe-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++20 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/1ed10f485dfe-1/llvm-project/libcxx-ci/build/generic-cxx20/projects/libcxx/std/iterators/predef.iterators/move.iterators/move.iterator/Output/iterator_concept_conformance.compile.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsyntax-only
310 mslibcxx CI C++20 > libcxx-trunk-shared.std/iterators/stream_iterators/istream_iterator::iterator_concept_conformance.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/1ed10f485dfe-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/stream.iterators/istream.iterator/iterator_concept_conformance.compile.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/1ed10f485dfe-1/llvm-project/libcxx-ci/build/generic-cxx20/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/1ed10f485dfe-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++20 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/1ed10f485dfe-1/llvm-project/libcxx-ci/build/generic-cxx20/projects/libcxx/std/iterators/stream.iterators/istream.iterator/Output/iterator_concept_conformance.compile.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsyntax-only
280 mslibcxx CI C++2b > libcxx-trunk-shared.std/iterators/predef_iterators/move_iterators/move_iterator::iterator_concept_conformance.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-tot /home/libcxx-builder/.buildkite-agent/builds/be36c4ce09c0-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/predef.iterators/move.iterators/move.iterator/iterator_concept_conformance.compile.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/be36c4ce09c0-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/be36c4ce09c0-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/be36c4ce09c0-1/llvm-project/libcxx-ci/build/generic-cxx2b/projects/libcxx/std/iterators/predef.iterators/move.iterators/move.iterator/Output/iterator_concept_conformance.compile.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsyntax-only
270 mslibcxx CI C++2b > libcxx-trunk-shared.std/iterators/stream_iterators/istream_iterator::iterator_concept_conformance.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-tot /home/libcxx-builder/.buildkite-agent/builds/be36c4ce09c0-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/stream.iterators/istream.iterator/iterator_concept_conformance.compile.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/be36c4ce09c0-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/be36c4ce09c0-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/be36c4ce09c0-1/llvm-project/libcxx-ci/build/generic-cxx2b/projects/libcxx/std/iterators/stream.iterators/istream.iterator/Output/iterator_concept_conformance.compile.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsyntax-only

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.


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


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>.


template<class In, class Out>

Quuxplusone requested changes to this revision.May 17 2021, 11:49 AM
Quuxplusone added inline comments.

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

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.


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


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

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

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


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.


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.


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


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

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.


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).


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

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.


ldionne added inline comments.Jun 14 2021, 1:20 PM
26 ↗(On Diff #351870)

This seems unused in this file.

36 ↗(On Diff #351870)

And so does this function.

40–43 ↗(On Diff #351870)


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?

142 ↗(On Diff #351870)

Should this be testing indirectly_movable or indirectly_movable_storable?

144 ↗(On Diff #351870)


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.
135 ↗(On Diff #351997)

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.

40–43 ↗(On Diff #351870)

Yes, that is more readable. Thanks :)

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.

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.

59 ↗(On Diff #351997)

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

ldionne added inline comments.Jun 21 2021, 10:01 AM
42–44 ↗(On Diff #353400)

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.