[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:82
+  using value_type = AssignableToBar;
+  Bar& operator*() const;
+};
----------------
Quuxplusone wrote:
> var-const wrote:
> > 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.
> > 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.
> 
> But those names aren't being used anywhere where that isn't already abundantly obvious (i.e., within 2 lines of the class definition). Contrariwise, the fact that some type is the `ValueType` of `InconsistentIteratorBar`, I think, is relevant information. Actually the presence of `Bar` in the name `InconsistentIteratorBar` is the thing that stands out right now: what is a `Bar`? There's no `Bar` in the global scope, no reason for any user to care about `Bar`. The only salient thing about `Bar` is that it's the value type of `InconsistentIterator[Bar]`... well, except that it's not. Which is why this test is weird. It's not a "proxy iterator", but it's also... well, what //is// it? If there's //no// sensible name for it, I'd just as soon name it `struct A`...
Ok, I decided to check these requirements exhaustively:
```
template<class _In, class _Out>
concept indirectly_copyable_storable =
  indirectly_writable<_Out, iter_value_t<_In>&> &&
  indirectly_writable<_Out, const iter_value_t<_In>&> &&
  indirectly_writable<_Out, iter_value_t<_In>&&> &&
  indirectly_writable<_Out, const iter_value_t<_In>&&> &&
  ...
```

This created 4 forms of `InconsistentIteratorFoo` and led me to rename them just to show the most interesting thing about them, like `NoConstRvalueAssignment`. Let me know what you think. I also applied your suggestion to use `ValueType` and `ReferenceType` as names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118432