[libcxx-commits] [PATCH] D118432: [libc++][ranges] Implement `indirectly_copyable{, _storable}`.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

In D118432#3281339 <https://reviews.llvm.org/D118432#3281339>, @var-const wrote:

> Note: I've also updated all the `iterator_concept_conformance` tests (under the assumption that for iterators, `indirectly_copyable{,_storable}` always behaves the same as `indirectly_movable{,_storable}`.

I have a patch locally (but apparently never got around to submitting it for review) that simply //removes// all of the "detail" concepts from every `iterator_concept_conformance.compile.pass.cpp`, leaving only the iterator concepts like `forward_iterator` and `bidirectional_iterator` (and only the relevant ones of //those//, even). I don't object to your expanding those tests, but I certainly wasn't going to ask you to do it.



================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable.compile.pass.cpp:54-55
+
+// Can move empty objects.
 static_assert( std::indirectly_movable<Empty*, Empty*>);
+
----------------
var-const wrote:
> Quuxplusone wrote:
> > FWIW, this test strikes me as redundant. There's nothing special about //empty// types in this context; changing the distracting name, this is really just testing that `struct A {};` is movable. Which of course it is; there's nothing special about class types as opposed to `int` in this context, either.
> > 
> > But, it might be nice to see ~10 lines of sanity tests for e.g. `indirectly_movable<void*, void*>`, `indirectly_movable<int*, void*>`,  `indirectly_movable<int(), int()>`, `indirectly_movable<int*, int()>`, `indirectly_movable<void, void>`, `indirectly_movable<int*, void>`... just to prove they don't explode.
> Agreed on both.
`Foo` is now unused (right?)


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp:26-29
+// Copying the underlying object between pointers (or dereferenceable classes) works. This is a non-exhaustive check
+// because this functionality comes from `indirectly_movable`.
+static_assert( std::indirectly_movable_storable<int*, int*>);
+static_assert( std::indirectly_movable_storable<const int*, int *>);
----------------
In the time it takes to explain that this check is non-exhaustive, you could have made it exhaustive. :)
```
static_assert( std::indirectly_movable_storable<int*, int*>);
static_assert( std::indirectly_movable_storable<const int*, int *>);
static_assert(!std::indirectly_movable_storable<int*, const int*>);
static_assert(!std::indirectly_movable_storable<const int*, const int *>);
static_assert( std::indirectly_movable_storable<int*, int[2]>);
static_assert(!std::indirectly_movable_storable<int[2], int*>);  // right?
```


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp:54
+static_assert( std::indirectly_writable<ProxyPointerFoo, std::iter_rvalue_reference_t<ProxyPointerFoo>>);
+static_assert(!std::indirectly_movable_storable<ProxyPointerFoo, ProxyPointerFoo>);
 
----------------
Here I think you could just test `!std::indirectly_movable_storable<FooConvertible*, Foo*>`, couldn't you? You've got the same weird pattern on lines 47-48 as on line 82 below, but even if you change it to `Foo operator*() const`, I don't see why you need `ProxyPointerFoo`'s value_type to be different from its actual `decay_t<decltype(*it)>`. This test seems just to be testing that if you have `In` and `Out` where `*Out = *In` isn't well-formed, then `indirectly_movable_storable` is `false`.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp:82
+  using value_type = AssignableToBar;
+  Bar& operator*() const;
+};
----------------
`ProxyPointerBar` seems like the wrong name for this type. It's basically just a `PointerTo<Bar>` — you dereference it, you get a reference to an existing `Bar` object — but it's just got the "wrong" `value_type`.
To make this more like a proxy iterator (like `vector<bool>::iterator`, for example), you should declare `Bar operator*() const;` without the ampersand. If you do that, does the test still pass? Does it still test whatever-is-being-tested-here?
(Strongly consider renaming `s/Bar/Reference/` and `s/AssignableToBar/ValueType/`.)


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp:99-101
   struct ReferenceType {
     operator CommonType&() const;
   };
----------------
Move this definition up to line 92, to eliminate the forward reference.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp:103-104
 
   using value_type = ValueType;
   ReferenceType& operator*() const;
 };
----------------
Same comment here as on line 82: it's highly unusual to have an iterator type whose `operator*` returns an `X&` but whose value type is `Y`. To make this a proxy iterator, change `ReferenceType&` to just `ReferenceType`. (And I'd drop the `Type`, for consistency with the name of the iterator trait `reference`.)


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp:126
+// to a type that can be assigned from anything.
+static_assert(!std::indirectly_movable_storable<NotConstructibleFromRefIn, AssignableFromAnything*>);
+
----------------
Let's add above this line
```
static_assert( std::indirectly_movable_storable<int*, AssignableFromAnything*>);
```
just to prove it's testing what we think it's testing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118432/new/

https://reviews.llvm.org/D118432