[libcxx-commits] [PATCH] D117660: [libc++] Fix a typo in reverse_iterator::operator=

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

Quuxplusone added a comment.

In D117660#3255829 <https://reviews.llvm.org/D117660#3255829>, @CaseyCarter wrote:

>> We should be checking is_assignable<It&, ...>. is_assignable<It, ...> checks for an rvalue left-hand side, which is basically never assignable-to.
>
> On the contrary, `is_assignable<T, U>` is almost always equal to `is_assignable<T&, U>` when `T` is a class type since assignment operators for class types are rarely `&`-qualified. I assume that's how this constraint slipped through testing, which indicates lack of test coverage for e.g. `reverse_iterator<int*>`.

It's subtler than that (thank goodness). If this `operator=` is incorrectly missing, then overload resolution ends up picking the copy assignment `operator=`, i.e. `x.operator=(y)` quietly becomes `x.operator=(X(y))`. And if `X` is `int*`, then I don't think that difference is observable... oh, except perhaps via the no-two-implicit-conversions-in-a-row "rule"... Well, I noodled around and got this far, but it still relies on ref-qualified `operator=`: https://godbolt.org/z/64v93nzPq

>> this is going to be an extra mess as soon as we need to support move-only iterators in reverse_iterator
>
> forward_iterator requires copyable. Move-only iterators are necessarily single-pass, and therefore never valid parameters for `reverse_iterator`.

Okay, I buy that. So at least the "extra mess" is not coming in C++20 or (almost certainly) C++23. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117660