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.
>
>
> >+ }
> >+
> >+ 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
> >
> >
>
>
