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

Fri Jan 28 10:21:48 PST 2022

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

I have many nits on the tests, but there's nothing terribly awful in there. It's great that you're finishing up these loose ends! 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;
+};
----------------
(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?

================
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*>);
----------------
I'm confused about this one. `CopyOnly(CopyOnly&&) = delete;`, so how is `indirectly_writable<_Out, iter_value_t<_In>&&>` satisfied?

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.copy/indirectly_copyable.compile.pass.cpp:80
+static_assert(!std::indirectly_copyable<int*, PointerTo<const int>>);
+static_assert( std::indirectly_copyable<PointerTo<const int>, int*>);
+static_assert(!std::indirectly_copyable<Empty*, PointerTo<int>>);
----------------
I recommend replacing line 80 with
```
static_assert( std::indirectly_copyable<PointerTo<int>, int*>);
static_assert(!std::indirectly_copyable<PointerTo<const int>, const int*>);
```
for better symmetry with lines 78–79. (If symmetry with those lines wasn't the point, then I don't see the point.)

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.copy/indirectly_copyable.subsumption.compile.pass.cpp:17
+
+#include <concepts>
+
----------------
This line can be removed AFAICT.

================
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;
+};
----------------
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`.

================
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;
----------------
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`.

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.copy/indirectly_copyable_storable.compile.pass.cpp:136-137
+
+// AssignableToBar can be constructed with a Bar and assigned to a Bar, so it does model indirectly_copyable_storable.
+using ProxyPointerBar = PointerTo<Bar, AssignableToBar>;
+static_assert( std::indirectly_copyable_storable<ProxyPointerBar, ProxyPointerBar>);
----------------
Same comment here as above: `PointerTo` should be a template of one parameter, `ProxyPointerBar` should be a struct not an alias, and `Bar`/`AssignableToBar` should be members of `ProxyPointerBar`.

================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.ind.copy/indirectly_copyable_storable.compile.pass.cpp:140
+
+struct CommonType { };
+
----------------
This should be a member of `NotConstructibleFromRefIn`.

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

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

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

================
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>);
+
----------------
Could you use `AssignableFromAnything*` here instead of `AssignableFromAnythingProxy`? I don't see that `AssignableFromAnythingProxy`'s struct-ness matters to this test.
Ditto line 167.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118432