[libc++][format] Move iterators when needed.

[Mordante]

Summary

LWG-3539 was already implemented but not marked as done.
LWG-3567 is implemented in this commit.

[• Quuxplusone]

I think this needs a test case. But I also assume that libc++ hasn't yet done a lot of the work to enable move-only iterators, is that right? How big is that work, and do you think it would be reasonable to block this PR until that work gets done (so that this PR could be updated to include a test case involving a move-only output iterator)?

libcxx/include/__format/format_context.h

103–104

This is correct per the Standard, but also kinda crazy, in that y = x.out() modifies the value of x. It should have been specified with an rvalue-ref-qualifier, like out() &&, right?
No action required, anyway, since this PR implements what the Standard says to do. Just kibitzing.

[Mordante]

I'll add tests. I think I can just use custom move only iterator and don't need to depend on move only iterators.
(In fact I expect the most common iterators for this class are copyable.)

libcxx/include/__format/format_context.h

103–104

I agree, this feels odd. But it's what the Standard mandates. I'm tempted to file an LWG-issue.

[Mordante]

I spend some time implementing tests but see some other issues, which I want to investigate before proceeding with this patch.

libcxx/include/__format/format_context.h

103–104

I've decided to file an LWG-issue.

[Mordante]

libcxx/include/__format/format_context.h

103–104

I was writing an LWG issue, however I see some issues. The function out is called in the requirements tables http://eel.is/c++draft/tab:formatter.basic, which would require a call to std::move, the same for http://eel.is/c++draft/format.context#example-1.

So it might be confusing only the out_ member is moved-from, but arg can be called.

Do you still feel it's worth the effort to file an issue?

[Mordante]

Rebased and a complete update of the patch.
Since parts of the format library now use ranges instead of pre C++20
algorithms internally it is possible to use move only iterators.

[Mordante]

Fixes CI.

[ldionne]

libcxx/test/std/utilities/format/format.formatter/format.context/format.context/advance_to.pass.cpp

48

Can we not use cpp20_output_iterator<std::back_insert_iterator<...>> and avoid defining our own move_only_push_back_iterator?

libcxx/test/std/utilities/format/format.functions/format_to.pass.cpp

33–34

This looks like a merge conflict to me. I think you meant to mirror the changes in format_to.locale.pass.cpp?

[Mordante]

Thanks for the review!

[Mordante]

Rebased and addresses review comments.

[Mordante]

Readds removed header.

libcxx/docs/Status/Cxx2bIssues.csv

 `3526 <https://wg21.link/LWG3526>`__,"Return types of ``uses_allocator_construction_args`` unspecified","June 2021","",""
 `3527 <https://wg21.link/LWG3527>`__,"``uses_allocator_construction_args`` handles rvalue pairs of rvalue references incorrectly","June 2021","",""
 `3528 <https://wg21.link/LWG3528>`__,"``make_from_tuple`` can perform (the equivalent of) a C-style cast","June 2021","",""
 `3529 <https://wg21.link/LWG3529>`__,"``priority_queue(first, last)`` should construct ``c`` with ``(first, last)``","June 2021","|Complete|","14.0"
 `3530 <https://wg21.link/LWG3530>`__,"``BUILTIN-PTR-MEOW`` should not opt the type out of syntactic checks","June 2021","",""
 `3532 <https://wg21.link/LWG3532>`__,"``split_view<V, P>::inner-iterator<true>::operator++(int)`` should depend on ``Base``","June 2021","","","|ranges|"
 `3533 <https://wg21.link/LWG3533>`__,"Make ``base() const &`` consistent across iterator wrappers that supports ``input_iterators``","June 2021","|Complete|","14.0","|ranges|"
 `3536 <https://wg21.link/LWG3536>`__,"Should ``chrono::from_stream()`` assign zero to duration for failure?","June 2021","","","|chrono|"
-`3539 <https://wg21.link/LWG3539>`__,"``format_to`` must not copy models of ``output_iterator<const charT&>``","June 2021","","","|format|"
+`3539 <https://wg21.link/LWG3539>`__,"``format_to`` must not copy models of ``output_iterator<const charT&>``","June 2021","|Complete|","14.0","|format|"
 `3540 <https://wg21.link/LWG3540>`__,"§[format.arg] There should be no const in ``basic_format_arg(const T* p)``","June 2021","|Complete|","14.0","|format|"
 `3541 <https://wg21.link/LWG3541>`__,"``indirectly_readable_traits`` should be SFINAE-friendly for all types","June 2021","|Complete|","14.0","|ranges|"
 `3542 <https://wg21.link/LWG3542>`__,"``basic_format_arg`` mishandles ``basic_string_view`` with custom traits","June 2021","|Complete|","14.0","|format|"
 `3543 <https://wg21.link/LWG3543>`__,"Definition of when ``counted_iterators`` refer to the same sequence isn't quite right","June 2021","|Nothing To Do|","","|ranges|"
 `3544 <https://wg21.link/LWG3544>`__,"``format-arg-store::args`` is unintentionally not exposition-only","June 2021","|Complete|","14.0","|format|"
 `3546 <https://wg21.link/LWG3546>`__,"``common_iterator``'s postfix-proxy is not quite right","June 2021","","","|ranges|"
 `3548 <https://wg21.link/LWG3548>`__,"``shared_ptr`` construction from ``unique_ptr`` should move (not copy) the deleter","June 2021","|Complete|","15.0"
 `3549 <https://wg21.link/LWG3549>`__,"``view_interface`` is overspecified to derive from ``view_base``","June 2021","|Complete|","14.0","|ranges|"

 `3535 <https://wg21.link/LWG3535>`__,"``join_view::iterator::iterator_category`` and ``::iterator_concept`` lie","October 2021","|Complete|","15.0","|ranges|"
 `3554 <https://wg21.link/LWG3554>`__,"``chrono::parse`` needs ``const charT*`` and ``basic_string_view<charT>`` overloads","October 2021","","","|chrono|"
 `3557 <https://wg21.link/LWG3557>`__,"The ``static_cast`` expression in ``convertible_to`` has the wrong operand","October 2021","|Complete|","14.0"
 `3559 <https://wg21.link/LWG3559>`__,"Semantic requirements of ``sized_range`` is circular","October 2021","|Nothing To Do|","","|ranges|"
 `3560 <https://wg21.link/LWG3560>`__,"``ranges::equal`` and ``ranges::is_permutation`` should short-circuit for ``sized_ranges``","October 2021","","","|ranges|"
 `3561 <https://wg21.link/LWG3561>`__,"Issue with internal counter in ``discard_block_engine``","October 2021","",""
 `3563 <https://wg21.link/LWG3563>`__,"``keys_view`` example is broken","October 2021","","","|ranges|"
 `3566 <https://wg21.link/LWG3566>`__,"Constraint recursion for ``operator<=>(optional<T>, U)``","October 2021","","","|spaceship|"
-`3567 <https://wg21.link/LWG3567>`__,"Formatting move-only iterators take two","October 2021","","","|format|"
+`3567 <https://wg21.link/LWG3567>`__,"Formatting move-only iterators take two","October 2021","|Complete|","16.0","|format|"
 `3568 <https://wg21.link/LWG3568>`__,"``basic_istream_view`` needs to initialize ``value_``","October 2021","|Complete|","16.0","|ranges|"
 `3570 <https://wg21.link/LWG3570>`__,"``basic_osyncstream::emit`` should be an unformatted output function","October 2021","",""
 `3571 <https://wg21.link/LWG3571>`__,"``flush_emit`` should set ``badbit`` if the ``emit`` call fails","October 2021","",""
 `3572 <https://wg21.link/LWG3572>`__,"``copyable-box`` should be fully ``constexpr``","October 2021","|Complete|","14.0","|ranges|"
 `3573 <https://wg21.link/LWG3573>`__,"Missing Throws element for ``basic_string_view(It begin, End end)``","October 2021","|Complete|","14.0"
 `3574 <https://wg21.link/LWG3574>`__,"``common_iterator`` should be completely ``constexpr``-able","October 2021","|Complete|","14.0","|ranges|"
 `3580 <https://wg21.link/LWG3580>`__,"``iota_view``'s ``iterator``'s binary ``operator+`` should be improved","October 2021","|Complete|","14.0","|ranges|"
 `3581 <https://wg21.link/LWG3581>`__,"The range constructor makes ``basic_string_view`` not trivially move constructible","October 2021","|Complete|","14.0","|ranges|"

libcxx/include/__format/buffer.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___FORMAT_BUFFER_H
 #define _LIBCPP___FORMAT_BUFFER_H
 
 #include <__algorithm/copy_n.h>
 #include <__algorithm/fill_n.h>
 #include <__algorithm/max.h>
 #include <__algorithm/min.h>
+#include <__algorithm/ranges_copy_n.h>
 #include <__algorithm/transform.h>
 #include <__algorithm/unwrap_iter.h>
 #include <__config>
 #include <__format/concepts.h>
 #include <__format/enable_insertable.h>
 #include <__format/format_to_n_result.h>
 #include <__iterator/back_insert_iterator.h>
 #include <__iterator/concepts.h>

 
 /// Write policy for copying the buffer to the output.
 template <class _OutIt, __fmt_char_type _CharT>
 class _LIBCPP_TEMPLATE_VIS __writer_iterator {
 public:
   _LIBCPP_HIDE_FROM_ABI explicit __writer_iterator(_OutIt __out_it)
       : __out_it_{_VSTD::move(__out_it)} {}
 
-  _LIBCPP_HIDE_FROM_ABI _OutIt __out_it() { return __out_it_; }
+  _LIBCPP_HIDE_FROM_ABI _OutIt __out_it() && { return std::move(__out_it_); }
 
   _LIBCPP_HIDE_FROM_ABI void __flush(_CharT* __ptr, size_t __n) {
-    __out_it_ = _VSTD::copy_n(__ptr, __n, _VSTD::move(__out_it_));
+    __out_it_ = std::ranges::copy_n(__ptr, __n, std::move(__out_it_)).out;
   }
 
 private:
   _OutIt __out_it_;
 };
 
 /// Concept to see whether a \a _Container is insertable.
 ///

libcxx/include/__format/format_context.h

public:
   }
 #ifndef _LIBCPP_HAS_NO_LOCALIZATION
   _LIBCPP_HIDE_FROM_ABI _VSTD::locale locale() {
     if (!__loc_)
       __loc_ = _VSTD::locale{};
     return *__loc_;
   }
 #endif
-  _LIBCPP_HIDE_FROM_ABI iterator out() { return __out_it_; }
-  _LIBCPP_HIDE_FROM_ABI void advance_to(iterator __it) { __out_it_ = __it; }
+  _LIBCPP_HIDE_FROM_ABI iterator out() { return std::move(__out_it_); }
+  _LIBCPP_HIDE_FROM_ABI void advance_to(iterator __it) { __out_it_ = std::move(__it); }
 
 private:
   iterator __out_it_;
   basic_format_args<basic_format_context> __args_;
 #ifndef _LIBCPP_HAS_NO_LOCALIZATION
 
   // The Standard doesn't specify how the locale is stored.
   // [format.context]/6

libcxx/test/std/utilities/format/format.formatter/format.context/format.context/advance_to.pass.cpp

 // void advance_to(iterator it);
 
 #include <format>
 #include <cassert>
 #include <string>
 
 #include "test_macros.h"
 #include "test_format_context.h"
+#include "test_iterators.h"
 
 template <class OutIt, class CharT>
 void test(
     std::basic_format_args<std::basic_format_context<OutIt, CharT>> args) {
   {
-    std::basic_string<CharT> str[3];
-    std::basic_format_context context = test_format_context_create(OutIt{str[0]}, args);
-    context.out() = CharT('a');
-    context.advance_to(OutIt{str[1]});
-    context.out() = CharT('b');
-    context.advance_to(OutIt{str[2]});
-    context.out() = CharT('c');
+    std::basic_string<CharT> str[3] = {
+        std::basic_string<CharT>{1}, std::basic_string<CharT>{1}, std::basic_string<CharT>{1}};
+    std::basic_format_context context = test_format_context_create(OutIt{str[0].begin()}, args);
+    *context.out()                    = CharT('a');
+    context.advance_to(OutIt{str[1].begin()});
+    *context.out() = CharT('b');
+    context.advance_to(OutIt{str[2].begin()});
+    *context.out() = CharT('c');
 
     assert(str[0].size() == 1);
     assert(str[0].front() == CharT('a'));
     assert(str[1].size() == 1);
     assert(str[1].front() == CharT('b'));
     assert(str[2].size() == 1);
     assert(str[2].front() == CharT('c'));
   }
 }
 
 void test() {
   test(std::basic_format_args(
-      std::make_format_args<std::basic_format_context<
-          std::back_insert_iterator<std::basic_string<char>>, char>>()));
+      std::make_format_args<
+          std::basic_format_context<cpp20_output_iterator<std::basic_string<char>::iterator>, char>>()));
 
 #ifndef TEST_HAS_NO_WIDE_CHARACTERS
   test(std::basic_format_args(
-      std::make_format_args<std::basic_format_context<
-          std::back_insert_iterator<std::basic_string<wchar_t>>, wchar_t>>()));
+      std::make_format_args<
+          std::basic_format_context<cpp20_output_iterator<std::basic_string<wchar_t>::iterator>, wchar_t>>()));
 #endif
 #ifndef TEST_HAS_NO_CHAR8_T
   test(std::basic_format_args(
-      std::make_format_args<std::basic_format_context<
-          std::back_insert_iterator<std::basic_string<char8_t>>, char8_t>>()));
+      std::make_format_args<
+          std::basic_format_context<cpp20_output_iterator<std::basic_string<char8_t>::iterator>, char8_t>>()));
 #endif
   test(std::basic_format_args(
-      std::make_format_args<std::basic_format_context<
-          std::back_insert_iterator<std::basic_string<char16_t>>,
-          char16_t>>()));
+      std::make_format_args<
+          std::basic_format_context<cpp20_output_iterator<std::basic_string<char16_t>::iterator>, char16_t>>()));
   test(std::basic_format_args(
-      std::make_format_args<std::basic_format_context<
-          std::back_insert_iterator<std::basic_string<char32_t>>,
-          char32_t>>()));
+      std::make_format_args<
+          std::basic_format_context<cpp20_output_iterator<std::basic_string<char32_t>::iterator>, char32_t>>()));
 }
 
 int main(int, char**) {
   test();
 
   return 0;
 }

libcxx/test/std/utilities/format/format.formatter/format.context/format.context/out.pass.cpp

 // iterator out();
 
 #include <format>
 #include <cassert>
 #include <string>
 
 #include "test_macros.h"
 #include "test_format_context.h"
+#include "test_iterators.h"
 
 template <class OutIt, class CharT>
 void test(
     std::basic_format_args<std::basic_format_context<OutIt, CharT>> args) {
   {
-    std::basic_string<CharT> str;
-    OutIt out_it{str};
-    std::basic_format_context context = test_format_context_create(out_it, args);
-    context.out() = CharT('a');
-    context.out() = CharT('b');
-    context.out() = CharT('c');
+    std::basic_string<CharT> str      = std::basic_string<CharT>(3, CharT(' '));
+    std::basic_format_context context = test_format_context_create(OutIt{str.begin()}, args);
+
+    // Note this operation is moving the iterator
+    OutIt out_it = context.out();
+    *out_it++    = CharT('a');
+    *out_it++    = CharT('b');
+    *out_it++    = CharT('c');
 
     assert(str.size() == 3);
     assert(str[0] == CharT('a'));
     assert(str[1] == CharT('b'));
     assert(str[2] == CharT('c'));
   }
 }
 
 void test() {
   test(std::basic_format_args(
-      std::make_format_args<std::basic_format_context<
-          std::back_insert_iterator<std::basic_string<char>>, char>>()));
+      std::make_format_args<
+          std::basic_format_context<cpp20_output_iterator<std::basic_string<char>::iterator>, char>>()));
+
 #ifndef TEST_HAS_NO_WIDE_CHARACTERS
   test(std::basic_format_args(
-      std::make_format_args<std::basic_format_context<
-          std::back_insert_iterator<std::basic_string<wchar_t>>, wchar_t>>()));
+      std::make_format_args<
+          std::basic_format_context<cpp20_output_iterator<std::basic_string<wchar_t>::iterator>, wchar_t>>()));
 #endif
 #ifndef TEST_HAS_NO_CHAR8_T
   test(std::basic_format_args(
-      std::make_format_args<std::basic_format_context<
-          std::back_insert_iterator<std::basic_string<char8_t>>, char8_t>>()));
+      std::make_format_args<
+          std::basic_format_context<cpp20_output_iterator<std::basic_string<char8_t>::iterator>, char8_t>>()));
 #endif
   test(std::basic_format_args(
-      std::make_format_args<std::basic_format_context<
-          std::back_insert_iterator<std::basic_string<char16_t>>,
-          char16_t>>()));
+      std::make_format_args<
+          std::basic_format_context<cpp20_output_iterator<std::basic_string<char16_t>::iterator>, char16_t>>()));
   test(std::basic_format_args(
-      std::make_format_args<std::basic_format_context<
-          std::back_insert_iterator<std::basic_string<char32_t>>,
-          char32_t>>()));
+      std::make_format_args<
+          std::basic_format_context<cpp20_output_iterator<std::basic_string<char32_t>::iterator>, char32_t>>()));
 }
 
 int main(int, char**) {
   test();
 
   return 0;
 }

libcxx/test/std/utilities/format/format.functions/format_to.locale.pass.cpp

 #include <cassert>
 #include <list>
 #include <vector>
 
 #include "test_macros.h"
 #include "format_tests.h"
 #include "string_literal.h"
 #include "test_format_string.h"
+#include "test_iterators.h"
 
 auto test =
     []<class CharT, class... Args>(
         std::basic_string_view<CharT> expected, test_format_string<CharT, Args...> fmt, Args&&... args) constexpr {
       {
         std::basic_string<CharT> out(expected.size(), CharT(' '));
         auto it = std::format_to(out.begin(), std::locale(), fmt, std::forward<Args>(args)...);
         assert(it == out.end());
         assert(out == expected);
       }
       {
         std::list<CharT> out;
         std::format_to(std::back_inserter(out), std::locale(), fmt, std::forward<Args>(args)...);
         assert(std::equal(out.begin(), out.end(), expected.begin(), expected.end()));
       }
       {
         std::vector<CharT> out;
         std::format_to(std::back_inserter(out), std::locale(), fmt, std::forward<Args>(args)...);
         assert(std::equal(out.begin(), out.end(), expected.begin(), expected.end()));
       }
       {
         assert(expected.size() < 4096 && "Update the size of the buffer.");
         CharT out[4096];
-        CharT* it = std::format_to(out, std::locale(), fmt, std::forward<Args>(args)...);
-        assert(std::distance(out, it) == int(expected.size()));
+        cpp20_output_iterator<CharT*> it =
+            std::format_to(cpp20_output_iterator{out}, std::locale(), fmt, std::forward<Args>(args)...);
+        assert(std::distance(out, base(it)) == int(expected.size()));
         // Convert to std::string since output contains '\0' for boolean tests.
-        assert(std::basic_string<CharT>(out, it) == expected);
+        assert(std::basic_string<CharT>(out, base(it)) == expected);
       }
     };
 
 auto test_exception = []<class CharT, class... Args>(std::string_view, std::basic_string_view<CharT>, Args&&...) {
   // After P2216 most exceptions thrown by std::format_to become ill-formed.
   // Therefore this tests does nothing.
   // A basic ill-formed test is done in format_to.locale.verify.cpp
   // The exceptions are tested by other functions that don't use the basic-format-string as fmt argument.

libcxx/test/std/utilities/format/format.functions/format_to.pass.cpp

 #include <cassert>
 #include <list>
 #include <vector>
 
 #include "test_macros.h"
 #include "format_tests.h"
 #include "string_literal.h"
 #include "test_format_string.h"
+#include "test_iterators.h"
 
 auto test =
     []<class CharT, class... Args>(
         std::basic_string_view<CharT> expected, test_format_string<CharT, Args...> fmt, Args&&... args) constexpr {
       {
         std::basic_string<CharT> out(expected.size(), CharT(' '));
         auto it = std::format_to(out.begin(), fmt, std::forward<Args>(args)...);
         assert(it == out.end());
         assert(out == expected);
       }
       {
         std::list<CharT> out;
         std::format_to(std::back_inserter(out), fmt, std::forward<Args>(args)...);
         assert(std::equal(out.begin(), out.end(), expected.begin(), expected.end()));
       }
       {
         std::vector<CharT> out;
         std::format_to(std::back_inserter(out), fmt, std::forward<Args>(args)...);
         assert(std::equal(out.begin(), out.end(), expected.begin(), expected.end()));
       }
       {
         assert(expected.size() < 4096 && "Update the size of the buffer.");
         CharT out[4096];
-        CharT* it = std::format_to(out, fmt, std::forward<Args>(args)...);
-        assert(std::distance(out, it) == int(expected.size()));
+        cpp20_output_iterator<CharT*> it = std::format_to(cpp20_output_iterator{out}, fmt, std::forward<Args>(args)...);
+        assert(std::distance(out, base(it)) == int(expected.size()));
         // Convert to std::string since output contains '\0' for boolean tests.
-        assert(std::basic_string<CharT>(out, it) == expected);
+        assert(std::basic_string<CharT>(out, base(it)) == expected);
       }
     };
 
 auto test_exception = []<class CharT, class... Args>(std::string_view, std::basic_string_view<CharT>, Args&&...) {
   // After P2216 most exceptions thrown by std::format become ill-formed.
   // Therefore this tests does nothing.
   // A basic ill-formed test is done in format.verify.cpp
   // The exceptions are tested by other functions that don't use the basic-format-string as fmt argument.