On Wed, 2 Jul 2025 at 10:33, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> 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.
Makes sense, thanks.
OK fo trunk with the redundant __formatter_chrono::_M_locale removed.
>>
>>
>> > {
>> > __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
>> >
>> >
>>
