On Thu, 4 Sept 2025 at 14:39, Tomasz Kaminski <[email protected]> wrote:
>
>
>
> On Thu, Sep 4, 2025 at 2:53 PM Tomasz Kamiński <[email protected]> wrote:
>>
>> When producing output, the libstdc++ format implementation only uses
>> _Sink_iter
>> specializations. Since users cannot construct basic_format_context, this is
>> the
>> only iterator type actually used. The __format_padded helper relies on this
>> property to efficiently pad sequences from tuples and ranges.
>>
>> However, the standard's formattable concept requires a generic format
>> function
>> in formatters that works with an any iterator type. This is intended to
>> future-proof the implementation by allowing new format_context types.
>> Previously,
>> libstdc++ used back_insert_iterator<basic_string<_CharT>> for this purpose.
>>
>> Normally, concept checks only instantiate function signatures, but with
>> user-defined formatters and deduced return types, the function body and all
>> called functions are instantiated. This could trigger a static assertion
>> error
>> in the range/tuple formatter that assumed the iterator was a _Sink_iter
>> (see included test).
>>
>> This patch resolves the issue by replacing the _Iter_for_t alias with the
>> internal _Drop_iter. This iterator sematnics is to drop elements, so
>> __format_padded can handle it by simply returning the input iterator, which
>> still produces the required behavior [1].
>>
>> An alternative of using _Sink_iter was considered but rejected because it
>> would
>> allow formatters to pass formattable requirements while only supporting
>> format_context and wformat_context, which seems counter to the design intent
>> (the std/format/formatter/concept.cc fails).
>>
>> [1] The standard's wording defines format functions as producing an output
>> representation, but does not explicitly require a formatter to be invoked
>> for each element. This allows the use of _Drop_iter to pass the concept check
>> without generating any output.
>>
>> PR libstdc++/121765
>>
>> libstdc++-v3/ChangeLog:
>>
>> * include/std/format (__format::_Drop_iter): Define.
>> (_Iter_for_t::type): Change alias to _Drop_iter.
>> (__format::__format_padded): Return __fc.out() for
>> _Drop_iter.
>> * testsuite/std/format/pr121765.cc: New test.
>> ---
>> The description of this patch is longer that then change, but I think
>> to record why I have choosen particular direction.
>>
>> Tested on x86_64-linux locally. OK for trunk?
>>
>> libstdc++-v3/include/std/format | 120 ++++++++++++------
>> libstdc++-v3/testsuite/std/format/pr121765.cc | 53 ++++++++
>> 2 files changed, 136 insertions(+), 37 deletions(-)
>> create mode 100644 libstdc++-v3/testsuite/std/format/pr121765.cc
>>
>> diff --git a/libstdc++-v3/include/std/format
>> b/libstdc++-v3/include/std/format
>> index d63c6fc9efd..e6377aaa2ed 100644
>> --- a/libstdc++-v3/include/std/format
>> +++ b/libstdc++-v3/include/std/format
>> @@ -56,7 +56,7 @@
>> #include <bits/ranges_base.h> // input_range, range_reference_t
>> #include <bits/ranges_util.h> // subrange
>> #include <bits/ranges_algobase.h> // ranges::copy
>> -#include <bits/stl_iterator.h> // back_insert_iterator, counted_iterator
>> +#include <bits/stl_iterator.h> // counted_iterator
>> #include <bits/stl_pair.h> // __is_pair
>> #include <bits/unicode.h> // __is_scalar_value, _Utf_view, etc.
>> #include <bits/utility.h> // tuple_size_v
>> @@ -110,10 +110,14 @@ namespace __format
>> template<typename _CharT>
>> class _Sink_iter;
>>
>> + // Output iterator that ignores the characters
>> + template<typename _CharT>
>> + class _Drop_iter;
>> +
>> // An unspecified output iterator type used in the `formattable` concept.
>> template<typename _CharT>
>> struct _Iter_for
>> - { using type = back_insert_iterator<basic_string<_CharT>>; };
>> + { using type = _Drop_iter<_CharT>; };
>>
>> template<typename _CharT>
>> using __format_context = basic_format_context<_Sink_iter<_CharT>,
>> _CharT>;
>> @@ -3135,6 +3139,43 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
>> /// @cond undocumented
>> namespace __format
>> {
>> + template<typename _CharT>
>> + class _Drop_iter
>> + {
>> + public:
>> + using iterator_category = output_iterator_tag;
>> + using value_type = void;
>> + using difference_type = ptrdiff_t;
>> + using pointer = void;
>> + using reference = void;
>> +
>> + _Drop_iter() = default;
>> + _Drop_iter(const _Drop_iter&) = default;
>> + _Drop_iter& operator=(const _Drop_iter&) = default;
>> +
>> + [[__gnu__::__always_inline__]]
>> + constexpr _Drop_iter&
>> + operator=(_CharT __c)
>> + { return *this; }
>> +
>> + [[__gnu__::__always_inline__]]
>> + constexpr _Drop_iter&
>> + operator=(basic_string_view<_CharT> __s)
>> + { return *this; }
>> +
>> + [[__gnu__::__always_inline__]]
>> + constexpr _Drop_iter&
>> + operator*() { return *this; }
>> +
>> + [[__gnu__::__always_inline__]]
>> + constexpr _Drop_iter&
>> + operator++() { return *this; }
>> +
>> + [[__gnu__::__always_inline__]]
>> + constexpr _Drop_iter
>> + operator++(int) { return *this; }
>> + };
>> +
>> template<typename _CharT>
>> class _Sink_iter
>> {
>> @@ -5503,42 +5544,47 @@ namespace __format
>> const _Spec<_CharT>& __spec,
>> _Callback&& __call)
>> {
>> - // This is required to implement formatting with padding,
>> - // as we need to format to temporary buffer, using the same iterator.
>> - static_assert(is_same_v<_Out, __format::_Sink_iter<_CharT>>);
>> -
>> - const size_t __padwidth = __spec._M_get_width(__fc);
>> - if (__padwidth == 0)
>> - return __call(__fc);
>> -
>> - struct _Restore_out
>> - {
>> - _Restore_out(basic_format_context<_Sink_iter<_CharT>, _CharT>& __fc)
>> - : _M_ctx(std::addressof(__fc)), _M_out(__fc.out())
>> - { }
>> -
>> - void
>> - _M_disarm()
>> - { _M_ctx = nullptr; }
>> -
>> - ~_Restore_out()
>> - {
>> - if (_M_ctx)
>> - _M_ctx->advance_to(_M_out);
>> - }
>> -
>> - private:
>> - basic_format_context<_Sink_iter<_CharT>, _CharT>* _M_ctx;
>> - _Sink_iter<_CharT> _M_out;
>> - };
>> + if constexpr (is_same_v<_Out, _Iter_for_t<_CharT>>)
>
> This would be better if we use _Drop_iter here. This is result of me doing
> some other tests.
Yes, I agree it should be _Drop_iter<_CharT>. That way if we ever
change the _Iter_for<C>::type to be _Iter_sink<C> this won't break, or
if we change it to some different type then we will have to explicitly
consider how to handle that type here (it's safe to ignore _Drop_iter,
but might not be for some other type).
Alternatively, we could just remove _Iter_for and define
_Iter_for_t<C> as type_identity_t<_Drop_iter<C>>, or just
_Drop_iter<C> directly?
>>
>> + return __fc.out();
>> + else
>> + {
>> + // This is required to implement formatting with padding,
>> + // as we need to format to temporary buffer, using the same
>> iterator.
>> + static_assert(is_same_v<_Out, _Sink_iter<_CharT>>);
>> +
>> + const size_t __padwidth = __spec._M_get_width(__fc);
>> + if (__padwidth == 0)
>> + return __call(__fc);
>> +
>> + struct _Restore_out
>> + {
>> + _Restore_out(basic_format_context<_Sink_iter<_CharT>, _CharT>&
>> __fc)
>> + : _M_ctx(std::addressof(__fc)), _M_out(__fc.out())
>> + { }
>> +
>> + void
>> + _M_disarm()
>> + { _M_ctx = nullptr; }
>> +
>> + ~_Restore_out()
>> + {
>> + if (_M_ctx)
>> + _M_ctx->advance_to(_M_out);
>> + }
>>
>> - _Restore_out __restore(__fc);
>> - _Padding_sink<_Sink_iter<_CharT>, _CharT> __sink(__fc.out(),
>> __padwidth);
>> - __fc.advance_to(__sink.out());
>> - __call(__fc);
>> - __fc.advance_to(__sink._M_finish(__spec._M_align, __spec._M_fill));
>> - __restore._M_disarm();
>> - return __fc.out();
>> + private:
>> + basic_format_context<_Sink_iter<_CharT>, _CharT>* _M_ctx;
>> + _Sink_iter<_CharT> _M_out;
>> + };
>> +
>> + _Restore_out __restore(__fc);
>> + _Padding_sink<_Sink_iter<_CharT>, _CharT> __sink(__fc.out(),
>> __padwidth);
>> + __fc.advance_to(__sink.out());
>> + __call(__fc);
>> + __fc.advance_to(__sink._M_finish(__spec._M_align, __spec._M_fill));
>> + __restore._M_disarm();
>> + return __fc.out();
>> + }
>> }
>>
>> template<size_t _Pos, typename _Tp, typename _CharT>
>> diff --git a/libstdc++-v3/testsuite/std/format/pr121765.cc
>> b/libstdc++-v3/testsuite/std/format/pr121765.cc
>> new file mode 100644
>> index 00000000000..1358fc11052
>> --- /dev/null
>> +++ b/libstdc++-v3/testsuite/std/format/pr121765.cc
>> @@ -0,0 +1,53 @@
>> +// { dg-do compile { target c++23 } }
>> +
>> +#include <format>
>> +#include <utility>
>> +
>> +struct MyPair
>> +{
>> + int x;
>> + int y;
>> +};
>> +
>> +template<typename CharT>
>> +struct std::formatter<MyPair, CharT>
>> +{
>> + template<typename ParseContext>
>> + auto parse(ParseContext& pc)
>> + { return _formatter.parse(pc); }
>> +
>> + template<typename FormatContext>
>> + auto format(const MyPair& mp, FormatContext& fc) const
>> + { return _formatter.format(std::make_pair(mp.x, mp.y), fc); }
>> +
>> +private:
>> + std::formatter<std::pair<int, int>, CharT> _formatter;
>> +};
>> +
>> +static_assert(std::formattable<MyPair, char>);
>> +static_assert(std::formattable<MyPair, wchar_t>);
>> +
>> +struct MyRange
>> +{
>> + int* begin;
>> + int* end;
>> +};
>> +
>> +template<typename CharT>
>> +struct std::formatter<MyRange, CharT>
>> +{
>> + template<typename ParseContext>
>> + auto parse(ParseContext& pc)
>> + { return _formatter.parse(pc); }
>> +
>> + template<typename FormatContext>
>> + auto format(const MyRange& mp, FormatContext& fc) const
>> + { return _formatter.format(std::span<int>(mp.begin, mp.end), fc); }
>> +
>> +private:
>> + std::formatter<std::span<int>, CharT> _formatter;
>> +};
>> +
>> +static_assert(std::formattable<MyRange, char>);
>> +static_assert(std::formattable<MyRange, wchar_t>);
>> +
>> --
>> 2.51.0
>>
