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

Fri Jan 28 18:50:19 PST 2022

var-const added a comment.

Very useful comments, thanks!

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.copy/indirectly_copyable.compile.pass.cpp:21-27
+struct MoveOnly {
+  MoveOnly(MoveOnly&&) = default;
+  MoveOnly(MoveOnly const&) = delete;
+  MoveOnly& operator=(MoveOnly&&) = default;
+  MoveOnly& operator=(MoveOnly const&) = delete;
+  MoveOnly() = default;
+};
----------------
Quuxplusone wrote:
> (1) From the placement, I wonder if `MoveOnly() = default;` was meant to say `~MoveOnly() = default;`.
> (2) Lines 23 and 25 are superfluous.
> (3) We //could// use `"test/support/MoveOnly.h"` instead; is it worth it?
1. Hmm, semantically it makes sense to define the default constructor (move/copy constructor inhibit the compiler-generated default constructor), but not the default destructor (which is pretty much always generated).

2. True -- I guess the original intent was to be more explicit. I don't have a strong preference.

3. I think so; it also sidesteps issues 1 and 2.

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.copy/indirectly_copyable.compile.pass.cpp:72
+// Can copy copy-only objects.
+static_assert( std::indirectly_copyable<CopyOnly*, CopyOnly*>);
+static_assert(!std::indirectly_copyable<CopyOnly*, const CopyOnly*>);
----------------
Quuxplusone wrote:
> I'm confused about this one. `CopyOnly(CopyOnly&&) = delete;`, so how is `indirectly_writable<_Out, iter_value_t<_In>&&>` satisfied?
`indirectly_copyable` only requires `indirectly_writable<_Out, iter_reference_t<_In>>`. Maybe you're thinking of `indirectly_copyable_storable`, which does require `indirectly_writable<_Out, iter_value_t<_In>&&> `?

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.copy/indirectly_copyable_storable.compile.pass.cpp:37-41
+template<class T, class ValueType = T>
+struct PointerTo {
+  using value_type = ValueType;
+  T& operator*() const;
+};
----------------
Quuxplusone wrote:
> The second template param is used on line 69 below. For simplicity, I'd rather see `template<class T> struct PointerTo` here, and a completely separate (non-template) `struct ProxyPointer` on line 69.
> I'm also currently puzzled by the name "proxy pointer" and the comment on lines 67–68: //how// is this like a proxy? But I think that will become clearer as you simplify. I believe both `Foo` and `FooConvertible` can become members of `struct ProxyPointer`, which will encapsulate them a bit better and perhaps also eliminate the need for the forward declaration of `FooConvertible`.
Done (it looks like the forward declaration is still necessary, unfortunately).

Regarding the name -- the idea was that it's a pointer that returns a proxy of the type it points to, rather than the type itself (similar to how `vector<bool>`'s `reference` type isn't the same as `value_type&`). So it's more like "a pointer that returns a proxy" rather than "a pointer that itself is a proxy".

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.copy/indirectly_copyable_storable.compile.pass.cpp:73-123
+struct DeletedCopyCtor {
+  DeletedCopyCtor(DeletedCopyCtor const&) = delete;
+  DeletedCopyCtor& operator=(DeletedCopyCtor const&) = default;
+};
+
+struct DeletedNonconstCopyCtor {
+  DeletedNonconstCopyCtor(DeletedNonconstCopyCtor const&) = default;
----------------
Quuxplusone wrote:
> I think these 50 lines are pointless; none of these types are even `movable`, so of course pointers to them won't be `indirectly_copyable`.
Actually, while none of these types are `copyable`, `DeletedNonconstCopyCtor` and `DeletedNonconstCopyAssignment` _are_ `movable`. Do you think this is an issue with `movable`?

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.copy/indirectly_copyable_storable.compile.pass.cpp:140
+
+struct CommonType { };
+
----------------
Quuxplusone wrote:
> This should be a member of `NotConstructibleFromRefIn`.
Done. Whether `basic_common_reference` is specialized still has no effect on the test.

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable.compile.pass.cpp:39-41
+// Can't move if either pointer is const.
+static_assert(!std::indirectly_movable<int*, const int*>);
+static_assert( std::indirectly_movable<const int*, int*>);
----------------
Quuxplusone wrote:
> The comment on line 39 is wrong (but also unnecessary).
> Line 41 is now redundant with line 37. I think all you really want is to replace lines 35–41 with
> ```
> static_assert( std::indirectly_movable<int*, int*>);
> static_assert(!std::indirectly_movable<int*, const int*>);
> static_assert( std::indirectly_movable<const int*, int*>);
> static_assert(!std::indirectly_movable<const int*, const int*>);
> ```
> with no comments necessary at all.
Thanks for catching this! I'd still keep the comment, though -- mostly because it helps me split the tests into logical groups visually.

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

================
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*>);
+
----------------
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.

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.move/indirectly_movable_storable.compile.pass.cpp:133
+// to a type that can be assigned from anything.
+static_assert(!std::indirectly_movable_storable<NotConstructibleFromRefIn, AssignableFromAnythingProxy>);
+
----------------
Quuxplusone wrote:
> Could you use `AssignableFromAnything*` here instead of `AssignableFromAnythingProxy`? I don't see that `AssignableFromAnythingProxy`'s struct-ness matters to this test.
> Ditto line 167.
Right -- I got too caught up in renamings and missed that it could just be a pointer. Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118432