On Thu, 12 Jun 2025 at 14:40, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> On Thu, Jun 12, 2025 at 3:22 PM Jonathan Wakely <jwak...@redhat.com> wrote:
>>
>> On 06/06/25 12:41 +0200, Tomasz Kamiński wrote:
>> >This patch change implementation of the formatters for sys_info and
>> >local_info,
>> >so they no longer delegate to operator<< for ostream in case of empty spec.
>> >As this types may be only formatted with empty chrono-spec (with padding),
>> >they now
>> >use a separate __formatter_chrono_info formatter.
>> >
>> >The __formatter_chrono_info formats sys_info using format_to call with
>> >format
>> >specifier extracted from corresponding operator<<, that now delegates to
>> >format
>> >with empty spec. For local_info we replicate functionality of the
>> >operator<<.
>> >The alignment and padding is handled using an _Padding_sink.
>> >
>> >libstdc++-v3/ChangeLog:
>> >
>> > * include/bits/chrono_io.h (__format::__formatter_chrono_info):
>> > Define.
>> > (std::formatter<chrono::sys_info, _CharT>)
>> > (std::formatter<chrono::local_inf, _CharT>): Delegate to
>> > __format::__formatter_chrono_info.
>> > (std::operator<<(basic_ostream<_CharT, _Traits>& const sys_info&)):
>> > Use format on sys_info with empty format spec.
>> >---
>> > libstdc++-v3/include/bits/chrono_io.h | 132 +++++++++++++++++++++++---
>> > 1 file changed, 117 insertions(+), 15 deletions(-)
>> >
>> >diff --git a/libstdc++-v3/include/bits/chrono_io.h
>> >b/libstdc++-v3/include/bits/chrono_io.h
>> >index 24c7dfb91c9..bbbae3d3064 100644
>> >--- a/libstdc++-v3/include/bits/chrono_io.h
>> >+++ b/libstdc++-v3/include/bits/chrono_io.h
>> >@@ -1948,6 +1948,104 @@ namespace __format
>> > }
>> > };
>> >
>> >+ template<typename _CharT>
>> >+ struct __formatter_chrono_info
>> >+ {
>> >+ constexpr typename basic_format_parse_context<_CharT>::iterator
>> >+ _M_parse(basic_format_parse_context<_CharT>& __pc)
>> >+ {
>> >+ auto __first = __pc.begin();
>> >+ auto __last = __pc.end();
>> >+
>> >+ _Spec<_CharT> __spec{};
>> >+
>> >+ auto __finished = [&] {
>> >+ if (__first == __last || *__first == '}')
>> >+ {
>> >+ _M_spec = __spec;
>> >+ return true;
>> >+ }
>> >+ return false;
>> >+ };
>> >+
>> >+ if (__finished())
>> >+ return __first;
>> >+
>> >+ __first = __spec._M_parse_fill_and_align(__first, __last);
>> >+ if (__finished())
>> >+ return __first;
>> >+
>> >+ __first = __spec._M_parse_width(__first, __last, __pc);
>> >+ if (__finished())
>> >+ return __first;
>> >+
>> >+ return __spec._M_parse_locale(__first, __last);
>>
>> It looks like if we reach this point, i.e. the L option is present,
>> then we don't do _M_spec = __spec?
>>
>> And do we want to cause parsing to fail if __finished() == false after
>> parsing the locale option? Otherwise doesn't it mean we will ignore
>> any garbage after the L in the format string?
>>
>> Ah no, I see you handle that in formatter::parse after calling
>> _M_f._M_parse.
>>
>> But I don't think it's true that the chrono-specs must be empty for
>> local_info and sys_info, consider:
>>
>> std::format("{:%n %t %%}", std::chrono::sys_info{})
>>
>> Does __formatter_chrono_info need to parse and store the chrono-specs,
>> and then handle that in its _M_format? Only whitespace characters are
>> valid, and if there's a non-empty chrono-specs then we don't use the
>> format arg at all, just loop over the chrono-specs and output the
>> relevant whitespace chars. And that loop can be [[unlikely]] because
>> I doubt anybody will ever do this!
>
> The only from [time.format] specification for sys_info and local_info
> available,
> is that empty spec gives equivalent output to the osteam operator. Yes,
> I agree that you can use sys_info to print whitespaces or %, but unless the
> standard
> will be explicit that we need to do that, I would prefer not supporting it.
> Even if we risk regression, and someone submit a bug, they will not be able
> to point in standard where this is required.
I don't think we can argue it's not required to work (currently,
unless we change it as a DR).
The standard shows:
template<class charT> struct formatter<chrono::sys_info, charT>;
template<class charT> struct formatter<chrono::local_info, charT>;
and it says what format-chrono-spec means, and it only says we can
reject a chrono-spec if the type doesn't provide the info needed, but
there is no info needed for "%n %t %%" so it should be valid for all
chrono types.
The standard doesn't seem to say much about how those types are
formatted, but for "%n %t %%" I don't think there's any ambiguity.
>>
>>
>>
>> >+ }
>> >+
>> >+ template<typename _Info, typename _Out>
>> >+ typename basic_format_context<_Out, _CharT>::iterator
>> >+ format(const _Info& __i,
>> >+ basic_format_context<_Out, _CharT>& __fc) const
>> >+ {
>> >+ const size_t __padwidth = _M_spec._M_get_width(__fc);
>> >+ if (__padwidth == 0)
>> >+ return _M_format_to(__fc.out(), __i);
>> >+
>> >+ _Padding_sink<_Out, _CharT> __sink(__fc.out(), __padwidth);
>> >+ _M_format_to(__sink.out(), __i);
>> >+ return __sink._M_finish(_M_spec._M_align, _M_spec._M_fill);
>> >+ }
>> >+
>> >+ private:
>> >+ template<typename _Out>
>> >+ _Out
>> >+ _M_format_to(_Out __out, const chrono::sys_info& __si) const
>> >+ {
>> >+ using _FmtStr = _Runtime_format_string<_CharT>;
>> >+ // n.b. only decimal separator is locale dependent for specifiers
>> >+ // used below, as sys_info uses seconds and minutes duration, the
>> >+ // output is locale-independent.
>> >+ constexpr auto* __fs
>> >+ = _GLIBCXX_WIDEN("[{0:%F %T},{1:%F
>> >%T},{2:%T},{3:%Q%q},{0:%Z}]");
>> >+ const chrono::local_seconds __lb(__si.begin.time_since_epoch());
>> >+ return std::format_to(std::move(__out), _FmtStr(__fs),
>> >+ chrono::local_time_format(__lb,
>> >&__si.abbrev),
>> >+ __si.end, __si.offset, __si.save);
>> >+ }
>> >+
>> >+ template<typename _Out>
>> >+ _Out
>> >+ _M_format_to(_Out __out, const chrono::local_info& __li) const
>> >+ {
>> >+ *__out = _Separators<_CharT>::_S_squares()[0];
>> >+ ++__out;
>> >+ if (__li.result == chrono::local_info::unique)
>> >+ __out = _M_format_to(std::move(__out), __li.first);
>> >+ else
>> >+ {
>> >+ basic_string_view<_CharT> __sv;
>> >+ if (__li.result == chrono::local_info::nonexistent)
>> >+ __sv =_GLIBCXX_WIDEN("nonexistent");
>> >+ else
>> >+ __sv = _GLIBCXX_WIDEN("ambiguous");
>> >+ __out = __format::__write(std::move(__out), __sv);
>> >+
>> >+ __sv = _GLIBCXX_WIDEN(" local time between ");
>> >+ __out = __format::__write(std::move(__out), __sv);
>> >+ __out = _M_format_to(std::move(__out), __li.first);
>> >+
>> >+ __sv = _GLIBCXX_WIDEN(" and ");
>> >+ __out = __format::__write(std::move(__out), __sv);
>> >+ __out = _M_format_to(std::move(__out), __li.second);
>> >+ }
>> >+ *__out = _Separators<_CharT>::_S_squares()[1];
>> >+ ++__out;
>> >+ return std::move(__out);
>> >+ }
>> >+
>> >+ _Spec<_CharT> _M_spec;
>> >+ };
>> >+
>> > } // namespace __format
>> > /// @endcond
>> >
>> >@@ -2438,16 +2536,22 @@ namespace __format
>> > {
>> > constexpr typename basic_format_parse_context<_CharT>::iterator
>> > parse(basic_format_parse_context<_CharT>& __pc)
>> >- { return _M_f._M_parse(__pc, __format::_ChronoParts{}); }
>> >+ {
>> >+ auto __res = _M_f._M_parse(__pc);
>> >+ if (__res == __pc.end() || *__res == '}')
>> >+ return __res;
>> >+ std::__throw_format_error("format error: invalid format-spec for "
>> >+ "std::chrono::sys_info");
>> >+ }
>> >
>> > template<typename _Out>
>> > typename basic_format_context<_Out, _CharT>::iterator
>> > format(const chrono::sys_info& __i,
>> > basic_format_context<_Out, _CharT>& __fc) const
>> >- { return _M_f._M_format(__i, __fc); }
>> >+ { return _M_f.format(__i, __fc); }
>> >
>> > private:
>> >- __format::__formatter_chrono<_CharT> _M_f;
>> >+ __format::__formatter_chrono_info<_CharT> _M_f;
>> > };
>> >
>> > template<__format::__char _CharT>
>> >@@ -2455,16 +2559,22 @@ namespace __format
>> > {
>> > constexpr typename basic_format_parse_context<_CharT>::iterator
>> > parse(basic_format_parse_context<_CharT>& __pc)
>> >- { return _M_f._M_parse(__pc, __format::_ChronoParts{}); }
>> >+ {
>> >+ auto __res = _M_f._M_parse(__pc);
>> >+ if (__res == __pc.end() || *__res == '}')
>> >+ return __res;
>> >+ std::__throw_format_error("format error: invalid format-spec for "
>> >+ "std::chrono::local_info");
>> >+ }
>> >
>> > template<typename _Out>
>> > typename basic_format_context<_Out, _CharT>::iterator
>> > format(const chrono::local_info& __i,
>> > basic_format_context<_Out, _CharT>& __fc) const
>> >- { return _M_f._M_format(__i, __fc); }
>> >+ { return _M_f.format(__i, __fc); }
>> >
>> > private:
>> >- __format::__formatter_chrono<_CharT> _M_f;
>> >+ __format::__formatter_chrono_info<_CharT> _M_f;
>> > };
>> > #endif
>> >
>> >@@ -3326,15 +3436,7 @@ namespace __detail
>> > basic_ostream<_CharT, _Traits>&
>> > operator<<(basic_ostream<_CharT, _Traits>& __os, const sys_info& __i)
>> > {
>> >- // n.b. only decimal separator is locale dependent for specifiers
>> >- // used below, as sys_info uses seconds and minutes duration, the
>> >- // output is locale-independent.
>> >- constexpr auto* __fs
>> >- = _GLIBCXX_WIDEN("[{0:%F %T},{1:%F %T},{2:%T},{3:%Q%q},{0:%Z}]");
>> >- local_seconds __lb(__i.begin.time_since_epoch());
>> >- __os << std::format(__fs, local_time_format(__lb, &__i.abbrev),
>> >- __i.end, __i.offset, __i.save);
>> >- return __os;
>> >+ return __os << std::format(__os.getloc(), _GLIBCXX_WIDEN("{}"), __i);
>> > }
>> >
>> > /// Writes a local_info object to an ostream in an unspecified format.
>> >--
>> >2.49.0
>> >
>> >
>>
