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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
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>);
 
----------------
Quuxplusone wrote:
> 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`.
(this is a pre-existing test)
I think what's being tested here is that `indirectly_movable_storable` requires (omitting irrelevant part) `indirectly_movable<In, Out>` and `indirectly_writable<Out, iter_value_t<In>>`. `indirectly_movable<In, Out`, in turn, requires `indirectly_writable<Out, iter_rvalue_reference<In>>`; in the end, it boils down to:
```
  indirectly_writable<Out, iter_value_t<In>> &&
  indirectly_writable<Out, iter_rvalue_reference_t<In>>;
```
So this test tries to set up the situation where only one of these requirements is true. I don't think `!std::indirectly_movable_storable<FooConvertible*, Foo*>` covers the same case:
```
static_assert(!std::indirectly_writable<Foo*, std::iter_rvalue_reference_t<FooConvertible*>>); // False
static_assert( std::indirectly_writable<ProxyPointerFoo, std::iter_rvalue_reference_t<ProxyPointerFoo>>); // True
```


================
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;
+};
----------------
Quuxplusone wrote:
> `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/`.)
> If you do that, does the test still pass?

No -- it's no longer `indirectly_writable`.

While I see your point about the `ProxyPointer` name, I think the fact that it has "wrong" `value_type` is crucial for the class (it's essentially the reason for its existence), and just `PointerTo` omits that. How about `InconsistentIterator`? (It's not really a pointer)

> Strongly consider renaming s/Bar/Reference/ and s/AssignableToBar/ValueType/
The current names are supposed to convey that `Bar` is a completely uninteresting class on its own and `AssignableToBar` is a class that can be assigned to `Bar`. `ValueType` and `Reference` obscure that -- in fact, just looking at them, I'd expect that `Reference` is an alias for `ValueType&`. So I'd rather keep the current names.


================
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;
 };
----------------
Quuxplusone wrote:
> 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`.)
I'd prefer to keep as is, at least for this patch (this is preexisting code).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118432