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

Tue Feb 1 01:49:28 PST 2022

var-const marked 7 inline comments as done.
var-const added inline comments.

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable.compile.pass.cpp:43
+
+// Can move from a pointer into an array but cannot move from an array.
 static_assert( std::indirectly_movable<int*, int[2]>);
----------------
Quuxplusone wrote:
> var-const wrote:
> > var-const wrote:
> > > Quuxplusone wrote:
> > > > You certainly //can// move from an array (element), so this comment is wrong.
> > > > My intuition says that the real hurdle here is that an array type is not an iterator type, so it's not `indirectly`-anything — it has no `iter_value_t`. (But then, an `optional` isn't an iterator either, and I think `optional`s are `indirectly`-fooable, so my intuitive explanation may be wrong.)
> > > I'm actually not sure about the behavior here.
> > > 
> > > Intuitively, it would seem to me that:
> > > - `indirectly_movable<int[2], int*>` _should_ work due to array-to-pointer decay. After all, just like you said, you can move from an array -- `int *ptr, arr[2]; *ptr = std::move(*arr);` is certainly valid;
> > > - in either case, I would expect this to be commutative -- i.e., `indirectly_movable<int*, int[2]>` and `indirectly_movable<int[2], int*>` should either both be true or both be false.
> > > 
> > > > My intuition says that the real hurdle here is that an array type is not an iterator type, so it's not `indirectly`-anything — it has no `iter_value_t`.
> > > 
> > > `iter_value_t` does in fact work on array types -- `static_assert(is_same_v<iter_value_t<int[2]>, int>);` is successful with both libc++ and libstdc++, at least.
> > > 
> > > Digging a little into this, it appears that for `indirectly_movable<int[2], int*>` is false because it is indirectly writable but not indirectly readable. Reducing as much as possible, it boils down to:
> > > ```
> > > template<class In>
> > > concept simplified_indirectly_readable =
> > >   requires(const In i) {
> > >     { *i } -> same_as<iter_reference_t<In>>;
> > >   };
> > > 
> > > static_assert( simplified_indirectly_readable<int*>);
> > > static_assert(!simplified_indirectly_readable<int[2]>);
> > > ```
> > > (there's also `{ ranges::iter_move(i) } -> same_as<iter_rvalue_reference_t<In>>;` failing for the same reason, so it can be ignored)
> > > 
> > > The reason this check:
> > > ```
> > > { *i } -> same_as<iter_reference_t<In>>
> > > ```
> > > works for pointers but fails for arrays comes down to template argument deduction. The `const In i` argument decays from array to pointer as expected; however:
> > > - when `In` is a pointer, the `const` in the signature "attaches" to the pointer itself, so that it is a const pointer to a non-const object (`int* const`);
> > > - when `In` is an array, the `const` in the signature "attaches" to the type referred to, so that it's a const array that decays to a non-const pointer to a const object (`const int*`).
> > > 
> > > For a quick illustration:
> > > ```
> > > template <class T>
> > > void Foo(const T x) {
> > >   static_assert(is_same_v<decltype(x), const int*>);
> > > }
> > > 
> > > template <class T>
> > > void Bar(const T x) {
> > >   static_assert(is_same_v<decltype(x), int* const>);
> > > }
> > > 
> > > void Test() {
> > >   int arr[2];
> > >   Foo<int[2]>(arr);
> > >   
> > >   int* ptr = nullptr;
> > >   Bar<int*>(ptr);
> > > }
> > > ```
> > > ([link](https://wandbox.org/permlink/vIKFC4fnW1sPbfzS))
> > > 
> > > It seems to me that this is pretty subtle and I wonder whether this is intentional -- basically, whether arrays are intentionally not `indirectly_readable`, or it's just a by-product of the implementation. If arrays are intentionally `not_readable`, it seems like they should be excluded in a more obvious way.
> > > 
> > > Let me know what you think (and if you see any errors in my reasoning).
> > @Quuxplusone Any thoughts on this?
> > @Quuxplusone Any thoughts on this?
> 
> Not particularly. As you say, "when `In` is a pointer, the const in the signature "attaches" to the pointer itself [...] when `In` is an array, the const in the signature "attaches" to the type referred to". It would probably be a bit nicer if the requires-expression's parameter list in https://eel.is/c++draft/iterator.concept.readable took `const In&` instead of `const In`; but that wouldn't change the outcome.
> 
> Anyway, the comment "cannot move from an array" is still wrong. Personally I'd solve the issue by eliminating all these comments; but if you really want keep the comment, I still think "an array is not an iterator" is a reasonable explanation.
I mean I'm not sure `indirectly_movable<int[2], int*>` is actually supposed to be `false`. I'll start a thread on the LWG reflector.

> Anyway, the comment "cannot move from an array" is still wrong. Personally I'd solve the issue by eliminating all these comments; but if you really want keep the comment, I still think "an array is not an iterator" is a reasonable explanation.

My intended meaning was "cannot indirectly-move from an array" (which is true because it's basically saying `indirectly_movable<array_type, pointer> == false` in natural language). I'm not sure that the problem is that an array is not an iterator -- at least iterator traits do consider it an iterator (due to decay). Removing the comment is tempting but feels like sidestepping the issue. All in all, I'd go with "Arrays are not considered indirectly movable-from".

================
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 *>);
----------------
Quuxplusone wrote:
> 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?
> ```
Added these tests (but I still don't consider these exhaustive, exhaustive would be completely copying the tests for `indirectly_movable`).

================
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*>);
+
----------------
Quuxplusone wrote:
> 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.
Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118432