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

[Mikhail Maltsev via Phabricator via libcxx-commits]

miyuki marked 2 inline comments as done.
miyuki added inline comments.


================
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);
----------------
Quuxplusone wrote:
> 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.
> Why is this not just move_ti = move_fi;
`assert(false);` would not work in constexpr context, so I used an explicit call of `operator=` as a workaround (it would fail at compile time without the fix in `reverse_iterator.h`). I found a simpler workaround for `assert(false)`, so now I can use `move_ti = move_fi;`.
> Orthogonally(?), if you want to check that this is really calling the "right" operator=
An assertion in the conversion constructor should be enough to test the fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113417