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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

You can fix the Apple-Clang CI failures by rebasing.
(Incidentally, I'm surprised that people don't rebase reflexively when reuploading a diff. I rebase like five times a day even when I'm //not// uploading. :D)



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118432