On Fri, Jun 27, 2025 at 11:23 AM Jonathan Wakely <jwak...@redhat.com> wrote:
> On Fri, 27 Jun 2025 at 10:10, Tomasz Kaminski <tkami...@redhat.com> wrote:
> >
> >
> >
> > On Fri, Jun 27, 2025 at 10:31 AM Tomasz Kamiński <tkami...@redhat.com>
> wrote:
> >>
> >> This patch adjust all internal std::format call inside of
> __formatter_chrono,
> >> to use runtime format stirng and thus avoid compile time checking of
> validity
> >> of the format string. Majority of cases are covered by calling newly
> introduced
> >> _S_empty_fs() function that returns __Runtime_format_string containing
> >> _S_empty_spec, instead of passing later directly.
> >>
> >> In case of _M_j we use _S_str_d3 function (extracted from _S_str_d2),
> eliminating
> >> call to std::format outside of unlikely scenario in which day of year
> is greater
> >> than 1000 (this may happen for year_month_day with month greater than
> 12). In
> >> consequence, outside of handling subseconds, we no longer delegate to
> std::format
> >> or construct temporary strings, when formatting chrono types with ok()
> values.
> >>
> >> PR libstdc++/110739
> >>
> >> libstdc++-v3/ChangeLog:
> >>
> >> * include/bits/chrono_io.h (__formatter_chrono::_S_empty_fs):
> Define.
> >> (__formatter_chrono::_S_str_d2): Use _S_str_d3 for 3+ digits.
> >> (__formatter_chrono::_S_str_d3): Extracted from _S_str_d2.
> >> (__formatter_chrono::_M_H_I, __formatter_chrono::_M_R_X):
> Replace
> >> _S_empty_spec with _S_empty_fs().
> >> (__formatter_chrono::_M_j): Likewise and use _S_str_d3 in common
> >> case.
> >> ---
> >>
> >>
> >
> > I do not think this buys us much, but I think it is worth doing anyway.
>
> I agree, I've never really liked doing recursive std::format calls
> inside formatters (obviously sometimes it's unavoidable, e.g. for
> custom duration rep types).
>
> > It also finishes my side goal, of getting rid of temporary strings, and
> using local buffers,
> > that I applied to other specifiers in previous commits.
> > Tested on x86_64-linux locally. The std/time* test passed with
> > -D_GLIBCXX_USE_CXX11_ABI=0 and -D_GLIBCXX_DEBUG.
>
> Just to be sure, it passes without those options too, right? :-)
>
> > OK for trunk?
>
> Please put the always_inline attribute after the comment on _S_str_d3.
>
It is placed before comment on every other function in the surrounding, I
will fix all of them.
>
> OK for trunk with that change, thanks.
>
>
> >
> >>
> >> libstdc++-v3/include/bits/chrono_io.h | 27 ++++++++++++++++++++++-----
> >> 1 file changed, 22 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/libstdc++-v3/include/bits/chrono_io.h
> b/libstdc++-v3/include/bits/chrono_io.h
> >> index bcfd51b9866..d6bc6c7cf2a 100644
> >> --- a/libstdc++-v3/include/bits/chrono_io.h
> >> +++ b/libstdc++-v3/include/bits/chrono_io.h
> >> @@ -873,6 +873,11 @@ namespace __format
> >> static constexpr const _CharT* _S_minus_empty_spec = _S_chars +
> 17;
> >> static constexpr const _CharT* _S_empty_spec = _S_chars + 18;
> >>
> >> + [[__gnu__::__always_inline__]]
> >> + static _Runtime_format_string<_CharT>
> >> + _S_empty_fs()
> >> + { return _Runtime_format_string<_CharT>(_S_empty_spec); }
> >> +
> >> // Return the formatting locale.
> >> template<typename _FormatContext>
> >> std::locale
> >> @@ -1411,7 +1416,7 @@ namespace __format
> >> __i = 12;
> >> }
> >> else if (__i >= 100) [[unlikely]]
> >> - return std::format_to(std::move(__out), _S_empty_spec, __i);
> >> + return std::format_to(std::move(__out), _S_empty_fs(), __i);
> >>
> >> return __format::__write(std::move(__out),
> _S_two_digits(__i));
> >> }
> >> @@ -1425,11 +1430,15 @@ namespace __format
> >> {
> >> // Decimal number of days, without padding.
> >> auto __d =
> chrono::floor<chrono::days>(__t._M_hours).count();
> >> - return std::format_to(std::move(__out), _S_empty_spec, __d);
> >> + return std::format_to(std::move(__out), _S_empty_fs(), __d);
> >> }
> >>
> >> - return std::format_to(std::move(__out),
> _GLIBCXX_WIDEN("{:03d}"),
> >> - __t._M_day_of_year.count());
> >> + auto __d = __t._M_day_of_year.count();
> >> + if (__d >= 1000) [[unlikely]]
> >> + return std::format_to(std::move(__out), _S_empty_fs(), __d);
> >> +
> >> + _CharT __buf[3];
> >> + return __format::__write(std::move(__out), _S_str_d3(__buf,
> __d));
> >> }
> >>
> >> template<typename _FormatContext>
> >> @@ -1534,7 +1543,7 @@ namespace __format
> >>
> >> if (__hi >= 100) [[unlikely]]
> >> {
> >> - __out = std::format_to(std::move(__out), _S_empty_spec,
> __hi);
> >> + __out = std::format_to(std::move(__out), _S_empty_fs(),
> __hi);
> >> __sv.remove_prefix(2);
> >> }
> >> else
> >> @@ -1772,7 +1781,15 @@ namespace __format
> >> {
> >> if (__n < 100) [[likely]]
> >> return _S_two_digits(__n);
> >> + return _S_str_d3(__buf, __n);
> >> + }
> >>
> >> + [[__gnu__::__always_inline__]]
> >> + // Returns decimal representation of __n, padded to 3 digits.
> >> + // Returned string_view points to __buf.
> >> + static basic_string_view<_CharT>
> >> + _S_str_d3(span<_CharT, 3> __buf, unsigned __n)
> >> + {
> >> _S_fill_two_digits(__buf.data(), __n / 10);
> >> __buf[2] = _S_chars[__n % 10];
> >> return __string_view(__buf.data(), 3);
> >> --
> >> 2.49.0
> >>
>
>
