On Wed, Jul 2, 2025 at 11:27 AM Jonathan Wakely <jwak...@redhat.com> wrote:
> On 01/07/25 16:54 +0200, Tomasz Kamiński wrote:
> >This patch lifts locale initialization from locale-specific handling
> methods
> >into _M_format_to function, and pass the locale by const reference.
> >To avoid unnecessary computation of locale::classic(), we use
> _Optional_locale,
> >and emplace into it only for localized formatting (_M_spec._M_localized)
> or if
> >chrono-spec contains locale specific specifiers
> (_M_spec._M_locale_specific).
> >The later is constructs locale::classic() in more cases that strictly
> necessary,
> >as only subset of locale specific specifiers (%a, %A, %b, %B, %c, %p, %r)
> needs
> >locale, while _M_locale_specific is also set for %x,%X and when O/E
> modifiers are
> >used. However, none of default outputs are affects, so I believe this is
> >acceptable.
> >
> >In _M_S we no longer guard querying of numpuct facet, with check that
> requires
> >potentially equally expensive construction of locale::classic. We also
> mark
> >localized path as unlikely.
> >
> >The _M_locale method is no longer used in __formatter_chrono, and thus was
> >moved to __formatter_duration.
> >
> >libstdc++-v3/ChangeLog:
> >
> > * include/bits/chrono_io.h (__formatter_chrono::_M_format_to):
> > Compute locale and pass it to specifiers method.
> > (__formatter_chrono::_M_a_A, __formatter_chrono::_M_b_B)
> > (__formatter_chrono::_M_c, __formatter_chrono::_M_p)
> > (__formatter_chrono::_M_r): Accept locale instead of format
> context.
> > (__formatter_chrono::_M_subsecs): Call __ctx.locale() directly,
> > instead of _M_locale and do not compare with locale::classic().
> > Add [[unlikely]] attributes.
> > (__formatter_chrono::_M_locale): Move to __formatter_duration.
> > (__formatter_duration::_M_locale): Moved from __formatter_chrono.
> >---
> >v2 updates the commit message text only in hope to make it more readable.
> >
> > libstdc++-v3/include/bits/chrono_io.h | 71 ++++++++++++++++-----------
> > 1 file changed, 43 insertions(+), 28 deletions(-)
> >
> >diff --git a/libstdc++-v3/include/bits/chrono_io.h
> b/libstdc++-v3/include/bits/chrono_io.h
> >index bcf9830fb9e..a25cb9ada01 100644
> >--- a/libstdc++-v3/include/bits/chrono_io.h
> >+++ b/libstdc++-v3/include/bits/chrono_io.h
> >@@ -964,10 +964,16 @@ namespace __format
> > return std::move(__out);
> > };
> >
> >+ _Optional_locale __loc;
> We could add:
>
> bool __loc_is_classic = false;
>
> >+ if (_M_spec._M_localized)
> >+ __loc = __fc.locale();
>
> and set it to __loc == locale::classic() here,
>
> >+ else if (_M_spec._M_locale_specific)
> >+ __loc = locale::classic();
>
> and set it to true here.
>
> >+
> > struct tm __tm{};
> > bool __use_locale_fmt = false;
> > if (_M_spec._M_localized && _M_spec._M_locale_specific)
> >- if (__fc.locale() != locale::classic())
> >+ if (__loc.value() != locale::classic())
>
> Then we could just test __loc_is_classic here. That would avoid a
> second call to classic() for the case where we explicitly set __loc to
> classic().
>
I thought about it, but I think that the number of calls to
locale::classic() remains the same,
and we call it only once in both implementations.
My reasoning, the above is performed only if _M_spec._M_localized is true,
in that
case we initialize __loc with __fc.locale(), and would perform that check
anyway.
So I do not think extracting bool is worth the complexity.
Or in other words, we never compare __loc with locale::classic() in case we
initialize
it with it.
>
> > {
> > __use_locale_fmt = true;
> >
> >@@ -1004,7 +1010,7 @@ namespace __format
> > {
> > _CharT __c = *__first++;
> > if (__use_locale_fmt && _S_localized_spec(__c, __mod))
> [[unlikely]]
> >- __out = _M_locale_fmt(std::move(__out), __fc.locale(),
> >+ __out = _M_locale_fmt(std::move(__out), __loc.value(),
> > __tm, __c, __mod);
> > else switch (__c)
> > {
> >@@ -1014,15 +1020,17 @@ namespace __format
> > break;
> > case 'a':
> > case 'A':
> >- __out = _M_a_A(__t._M_weekday, std::move(__out), __fc,
> __c == 'A');
> >+ __out = _M_a_A(__t._M_weekday, std::move(__out),
> >+ __loc.value(), __c == 'A');
> > break;
> > case 'b':
> > case 'h':
> > case 'B':
> >- __out = _M_b_B(__t._M_month, std::move(__out), __fc, __c
> == 'B');
> >+ __out = _M_b_B(__t._M_month, std::move(__out),
> >+ __loc.value(), __c == 'B');
> > break;
> > case 'c':
> >- __out = _M_c(__t, std::move(__out), __fc);
> >+ __out = _M_c(__t, std::move(__out), __loc.value());
> > break;
> > case 'C':
> > case 'y':
> >@@ -1058,7 +1066,7 @@ namespace __format
> > __out = _M_M(__t._M_minutes, __print_sign());
> > break;
> > case 'p':
> >- __out = _M_p(__t._M_hours, std::move(__out), __fc);
> >+ __out = _M_p(__t._M_hours, std::move(__out),
> __loc.value());
> > break;
> > case 'q':
> > __out = _M_q(__t._M_unit_suffix, std::move(__out));
> >@@ -1067,7 +1075,7 @@ namespace __format
> > __out = _M_Q(__t, __print_sign(), __fc);
> > break;
> > case 'r':
> >- __out = _M_r(__t, __print_sign(), __fc);
> >+ __out = _M_r(__t, __print_sign(), __loc.value());
> > break;
> > case 'R':
> > case 'X':
> >@@ -1146,10 +1154,10 @@ namespace __format
> > return std::move(__out);
> > }
> >
> >- template<typename _OutIter, typename _FormatContext>
> >+ template<typename _OutIter>
> > _OutIter
> > _M_a_A(chrono::weekday __wd, _OutIter __out,
> >- _FormatContext& __ctx, bool __full) const
> >+ const locale& __loc, bool __full) const
> > {
> > // %a Locale's abbreviated weekday name.
> > // %A Locale's full weekday name.
> >@@ -1165,7 +1173,6 @@ namespace __format
> > __string_view(_GLIBCXX_WIDEN(" is not a valid
> weekday")));
> > }
> >
> >- locale __loc = _M_locale(__ctx);
> > const auto& __tp = use_facet<__timepunct<_CharT>>(__loc);
> > const _CharT* __days[7];
> > if (__full)
> >@@ -1176,10 +1183,10 @@ namespace __format
> > return _M_write(std::move(__out), __loc, __str);
> > }
> >
> >- template<typename _OutIter, typename _FormatContext>
> >+ template<typename _OutIter>
> > _OutIter
> > _M_b_B(chrono::month __m, _OutIter __out,
> >- _FormatContext& __ctx, bool __full) const
> >+ const locale& __loc, bool __full) const
> > {
> > // %b Locale's abbreviated month name.
> > // %B Locale's full month name.
> >@@ -1195,7 +1202,6 @@ namespace __format
> > __string_view(_GLIBCXX_WIDEN(" is not a valid
> month")));
> > }
> >
> >- locale __loc = _M_locale(__ctx);
> > const auto& __tp = use_facet<__timepunct<_CharT>>(__loc);
> > const _CharT* __months[12];
> > if (__full)
> >@@ -1206,17 +1212,17 @@ namespace __format
> > return _M_write(std::move(__out), __loc, __str);
> > }
> >
> >- template<typename _OutIter, typename _FormatContext>
> >+ template<typename _OutIter>
> > _OutIter
> > _M_c(const _ChronoData<_CharT>& __t, _OutIter __out,
> >- _FormatContext& __ctx) const
> >+ const locale& __loc) const
> > {
> > // %c Locale's date and time representation, for C-locale: %a
> %b %e %T %Y
> > // %Ec Locale's alternate date and time representation, for
> C-locale same as above
> >
> >- __out = _M_a_A(__t._M_weekday, std::move(__out), __ctx, false);
> >+ __out = _M_a_A(__t._M_weekday, std::move(__out), __loc, false);
> > *__out = _S_space;
> >- __out = _M_b_B(__t._M_month, std::move(++__out), __ctx, false);
> >+ __out = _M_b_B(__t._M_month, std::move(++__out), __loc, false);
> > *__out = _S_space;
> > __out = _M_d_e(__t._M_day, std::move(++__out), 'e');
> > *__out = _S_space;
> >@@ -1458,16 +1464,15 @@ namespace __format
> > return __format::__write(std::move(__out), _S_two_digits(__i));
> > }
> >
> >- template<typename _OutIter, typename _FormatContext>
> >+ template<typename _OutIter>
> > _OutIter
> >- _M_p(chrono::hours __h, _OutIter __out, _FormatContext& __ctx)
> const
> >+ _M_p(chrono::hours __h, _OutIter __out, const locale& __loc) const
> > {
> > // %p The locale's equivalent of the AM/PM designations.
> > auto __hi = __h.count();
> > if (__hi >= 24) [[unlikely]]
> > __hi %= 24;
> >
> >- locale __loc = _M_locale(__ctx);
> > const auto& __tp = use_facet<__timepunct<_CharT>>(__loc);
> > const _CharT* __ampm[2];
> > __tp._M_am_pm(__ampm);
> >@@ -1491,10 +1496,10 @@ namespace __format
> > return std::vformat_to(std::move(__out), _S_empty_spec,
> __t._M_ereps);
> > }
> >
> >- template<typename _OutIter, typename _FormatContext>
> >+ template<typename _OutIter>
> > _OutIter
> > _M_r(const _ChronoData<_CharT>& __t, _OutIter __out,
> >- _FormatContext& __ctx) const
> >+ const locale& __loc) const
> > {
> > // %r Locale's 12-hour clock time, for C-locale: %I:%M:%S %p
> > auto __hi = __t._M_hours.count() % 12;
> >@@ -1511,7 +1516,7 @@ namespace __format
> >
> > __string_view __sv(__buf, 9);
> > __out = __format::__write(std::move(__out), __sv);
> >- return _M_p(__t._M_hours, std::move(__out), __ctx);
> >+ return _M_p(__t._M_hours, std::move(__out), __loc);
> > }
> >
> > template<typename _OutIter>
> >@@ -1575,9 +1580,9 @@ namespace __format
> > return __out;
> >
> > _CharT __dot = _S_dot;
> >- if (_M_spec._M_localized)
> >- if (auto __loc = __ctx.locale(); __loc != locale::classic())
> >+ if (_M_spec._M_localized) [[unlikely]]
> > {
> >+ auto __loc = __ctx.locale();
> > const auto& __np = use_facet<numpunct<_CharT>>(__loc);
> > __dot = __np.decimal_point();
> > }
> >@@ -1587,8 +1592,8 @@ namespace __format
> > if (_M_spec._M_floating_point_rep)
> > {
> > _Str_sink<_CharT> __sink;
> >- if (_M_spec._M_localized && _M_spec._M_custom_rep)
> >- std::vformat_to(__sink.out(), _M_locale(__ctx),
> >+ if (_M_spec._M_localized && _M_spec._M_custom_rep)
> [[unlikely]]
> >+ std::vformat_to(__sink.out(), __ctx.locale(),
> > _GLIBCXX_WIDEN("{1:0.{2}Lf}"),
> __t._M_ereps);
> > else
> > std::vformat_to(__sink.out(),
> >@@ -1846,7 +1851,6 @@ namespace __format
> > };
> >
> > using __formatter_chrono<_CharT>::__formatter_chrono;
> >- using __formatter_chrono<_CharT>::_M_locale;
> > using __formatter_chrono<_CharT>::_M_spec;
> >
> > template<typename _Duration, typename _ParseContext>
> >@@ -1876,6 +1880,17 @@ namespace __format
> > return __res;
> > }
> >
> >+ // Return the formatting locale.
> >+ template<typename _FormatContext>
> >+ std::locale
> >+ _M_locale(_FormatContext& __fc) const
> >+ {
> >+ if (!_M_spec._M_localized)
> >+ return std::locale::classic();
> >+ else
> >+ return __fc.locale();
> >+ }
>
> It looks like you've copied this to __formatter_duration but not
> removed it from __formatter_chrono, so it's duplicated now (and not
> needed in __formatter_chrono).
>
> >+
> > // Format duration for empty chrono-specs, e.g. "{}" (C++20
> [time.format] p6).
> > template<typename _Rep, typename _Period, typename _FormatContext>
> > typename _FormatContext::iterator
> >--
> >2.49.0
> >
> >
>
>
