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

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.

LGTM mod a few last comments on the test.



================
Comment at: libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp:50-53
+        bool f = false;
+        // assert(false) will not work in constexpr context, so we use a
+        // variable to workaround this issue
+        assert(f);
----------------
I believe a better fix would be to remove `TEST_CONSTEXPR_CXX17` from this function:
```
ToIter(char *) { assert(false); }
```
This function should never be called at all. You could even leave off the body if you want:
```
ToIter(char *);
```
(But you can't `=delete` it, because the library is checking that `is_convertible_v<char*, ToIter>`.)


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp:43-48
+    TEST_CONSTEXPR_CXX17 ToIter(char *src) : m_value(src) {
+        bool f = false;
+        // assert(false) will not work in constexpr context, so we use a
+        // variable to workaround this issue
+        assert(f);
+    }
----------------
Ditto here: just `ToIter(char *);` should suffice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113417