[libcxx-commits] [PATCH] D117396: [RFC][libc++] Allow non-copyable OutputIterators.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__config:1084-1094
+// In C++17 (and earlier) an iterator is required to be CopyConstructible and
+// CopyAssignable, but not MoveConstructibe nor MoveAssignable.
+// In C++20 the concepts input_iterator and output_iterator require movable but
+// not copyable. (forward_iterator requires copyable.)
+// Depending on the standard used some algorithms need to be adapted, for
+// example fill_n calls __fill_n with an OutputIterator argument.
+#if _LIBCPP_STD_VER > 17
----------------
You don't need this; you should `_VSTD::move(__it)` unconditionally. Even in C++11, if a type `It` supports
```
it = jt;  // OK
it = It(jt);  // compiler error
```
then it is not an iterator type. Iterators have to be copyable, and that includes movable as a subcondition.
Also, moving is never slower than copying. If C++20 forces us to update some copies to moves, we should do the upgrade unconditionally. (Even in C++03, where `_VSTD::move` is supported as an extension.)


================
Comment at: libcxx/test/support/test_iterators.h:35-42
+#if TEST_STD_VER > 17
+    // Starting with C++20 an output_iterator must be movable and doesn't
+    // require copyable.
+    output_iterator(const output_iterator&) = delete;
+    output_iterator(output_iterator&&) = default;
+    output_iterator& operator=(const output_iterator&) = delete;
+    output_iterator& operator=(output_iterator&&) = default;
----------------
This is inconsistent with the tactic we picked for `input_iterator`: We now have a `cpp17_input_iterator<T*>` and a `cpp20_input_iterator<T*>` as two differently named types, and we test them both in C++20 mode.
Should we just use this (much simpler) tactic for both `output_iterator` and `input_iterator`?
If there's a good reason //not// to use this tactic for `input_iterator`, then, does that same good reason apply to `output_iterator`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117396