[libcxx-commits] [PATCH] D113417: [libcxx] Fix enable_if condition of std::reverse_iterator::operator=

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp:40
 
+typedef char *FromIter;
+
----------------
I recommend eliminating this typedef. Right now there's a symmetry in the //names// of `FromIter` and `ToIter` but actually the former is not a class type.


================
Comment at: libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp:73
+    std::move_iterator<ToIter> move_ti;
+    move_ti.operator=<>(move_fi);
+    assert(move_ti.base().m_value == fi);
----------------
Why is this not just `move_ti = move_fi;`?
What you've got won't necessarily work on non-libc++ implementations.
Orthogonally(?), if you want to check that this is really calling the "right" `operator=` instead of doing an implicit conversion followed by the "wrong" `operator=`, then you could make those two operators do different things; e.g.
```
    ToIter(const FromIter& src) { assert(false); }
    TEST_CONSTEXPR_CXX17 ToIter& operator=(const FromIter& src) { m_value = 1; return *this; }
    TEST_CONSTEXPR_CXX17 ToIter& operator=(FromIter&& src) { m_value = 1; return *this; }
    [...]
    move_ti = move_fi;
    assert(move_ti.m_value == 1);
    move_ti = std::move(move_fi);
    assert(move_ti.m_value == 2);
```
Ditto in the `reverse_iterator` test below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113417