On Mon, 7 Jul 2025 at 10:49, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> On Mon, Jul 7, 2025 at 11:44 AM Jonathan Wakely <jwak...@redhat.com> wrote:
>>
>> On Mon, 7 Jul 2025 at 10:21, Tomasz Kamiński <tkami...@redhat.com> wrote:
>> >
>> > From: XU Kailiang <xu2k...@outlook.com>
>> >
>> > C++ formatting locale could have a custom time_put that performs
>> > differently from the C locale, so do not use __timepunct directly,
>> > instead all of above specifiers use _M_locale_fmt.
>> >
>> > For %a/%A/%b/%h/%B, the code handling the exception is now moved
>> > to the _M_check_ok function, that is inovked before handling of the
>>
>> "invoked"
>>
>> > conversion specifier. For time_points the values of months/weekday
>> > are computed, and thus are always ok(), this information is indicated
>> > by new _M_time_point member of the _ChronoSpec.
>> >
>> > The different handling of j specifier for durations and time_points/
>> > calendar types, is now handled using only _ChronoParts, and _M_time_only
>> > _ChronoSpec is no longer needed, thus is was removed.
>>
>> I think this makes the handling for durations a bit easier to
>> understand, thanks.
>>
>> >
>> > PR libstdc++/117214
>> >
>> > libstdc++-v3/ChangeLog:
>> >
>> > * include/bits/chrono_io.h (_ChronoSpec::_M_time_only): Remove.
>> > (_ChronoSpec::_M_time_point): Define.
>> > (__formatter_chrono::_M_parse): Use __parts to determine
>> > interpretation of j.
>> > (__formatter_chrono::_M_check_ok): Define.
>> > (__formatter_chrono::_M_format_to): Invoke _M_check_ok.
>> > (__formatter_chrono::_M_a_A, __formatter_chrono::_M_b_B): Move
>> > exception throwing to _M_check_ok.
>> > (__formatter_chrono::_M_j): Use _M_needs to define interpreation.
>>
>> "interpretation"
>>
>> > (__formatter_duration::_S_spec_for): Set _M_time_point.
>> > * testsuite/std/time/format/pr117214_custom_timeput.cc: New
>> > test.
>> >
>> > Co-authored-by: Tomasz Kaminski <tkami...@redhat.com>
>> > Signed-off-by: XU Kailiang <xu2k...@outlook.com>
>> > Signed-off-by: Tomasz Kaminski <tkami...@redhat.com>
>> > ---
>> > This patchs adjust the implementation as follows:
>> > * we use _M_locale_fmt for all specifiers
>> > * %h which is alias for %b is also covered
>> >
>> > Tested on x86_64-linux localy.
>> >
>> > libstdc++-v3/include/bits/chrono_io.h | 58 ++++++++++++++-----
>> > .../time/format/pr117214_custom_timeput.cc | 37 ++++++++++++
>> > 2 files changed, 81 insertions(+), 14 deletions(-)
>> > create mode 100644
>> > libstdc++-v3/testsuite/std/time/format/pr117214_custom_timeput.cc
>> >
>> > diff --git a/libstdc++-v3/include/bits/chrono_io.h
>> > b/libstdc++-v3/include/bits/chrono_io.h
>> > index 72cd569ccd6..863b3550e4f 100644
>> > --- a/libstdc++-v3/include/bits/chrono_io.h
>> > +++ b/libstdc++-v3/include/bits/chrono_io.h
>> > @@ -280,8 +280,8 @@ namespace __format
>> > // in the format-spec, e.g. "{:L%a}" is localized and
>> > locale-specific,
>> > // but "{:L}" is only localized and "{:%a}" is only locale-specific.
>> > unsigned _M_locale_specific : 1;
>> > - // Indicates that we are handling duration.
>> > - unsigned _M_time_only : 1;
>> > + // Indicates that we are handling time_point.
>> > + unsigned _M_time_point : 1;
>> > // Indicates that duration should be treated as floating point.
>> > unsigned _M_floating_point_rep : 1;
>> > // Indicate that duration uses user-defined representation.
>> > @@ -693,8 +693,10 @@ namespace __format
>> > __allowed_mods = _Mod_O;
>> > break;
>> > case 'j':
>> > - __needed = __spec._M_time_only ? _HoursMinutesSeconds
>> > - : _DayOfYear;
>> > + __needed = __parts & _DayOfYear;
>> > + // 'j' is decimal number of days for durations
>> > + if (__needed == _None)
>> > + __needed = _HoursMinutesSeconds;
>>
>> Maybe it's because I haven't slept well, but I found the comment here
>> didn't make the logic clearer for me (why is __needed = _None a
>> duration? what does HMS have to do with days?).
>> Would this be better?
>>
>> // If we do not know day-of-year then we must have a
>> duration,
>> // which is to be formatted as decimal number of days.
>
> Yes, that makes sense. I will put that wording there.
> Given that the rest of comments are only typos, are changes otherwise OK for
> trunk?
> Or are you still reviewing it?
No, I reviewed the rest and it's OK for trunk - thanks (and thanks to
Kailiang for the original patch).
>>
>>
>>
>> > break;
>> > case 'm':
>> > __needed = _Month;
>> > @@ -919,7 +921,13 @@ namespace __format
>> > {
>> > switch (__conv)
>> > {
>> > + case 'a':
>> > + case 'A':
>> > + case 'b':
>> > + case 'B':
>> > case 'c':
>> > + case 'h':
>> > + case 'p':
>> > case 'r':
>> > case 'x':
>> > case 'X':
>> > @@ -947,6 +955,32 @@ namespace __format
>> > return __out;
>> > }
>> >
>> > + void
>> > + _M_check_ok(const _ChronoData<_CharT>& __t, _CharT __conv) const
>> > + {
>> > + // n.b. for time point all date parts are computed, so
>> > + // they are alwas ok.
>>
>> "always"
>>
>> > + if (_M_spec._M_time_point || _M_spec._M_debug)
>> > + return;
>> > +
>> > + switch (__conv)
>> > + {
>> > + case 'a':
>> > + case 'A':
>> > + if (!__t._M_weekday.ok()) [[unlikely]]
>> > + __throw_format_error("format error: invalid weekday");
>> > + return;
>> > + case 'b':
>> > + case 'h':
>> > + case 'B':
>> > + if (!__t._M_month.ok()) [[unlikely]]
>> > + __throw_format_error("format error: invalid month");
>> > + return;
>> > + default:
>> > + return;
>> > + }
>> > + }
>> > +
>> > template<typename _OutIter, typename _FormatContext>
>> > _OutIter
>> > _M_format_to(const _ChronoData<_CharT>& __t, _OutIter __out,
>> > @@ -1003,6 +1037,8 @@ namespace __format
>> > do
>> > {
>> > _CharT __c = *__first++;
>> > + _M_check_ok(__t, __c);
>> > +
>> > if (__use_locale_fmt && _S_localized_spec(__c, __mod))
>> > [[unlikely]]
>> > __out = _M_locale_fmt(std::move(__out), __fc.locale(),
>> > __tm, __c, __mod);
>> > @@ -1153,11 +1189,8 @@ namespace __format
>> > {
>> > // %a Locale's abbreviated weekday name.
>> > // %A Locale's full weekday name.
>> > - if (!__wd.ok())
>> > + if (_M_spec._M_debug && !__wd.ok())
>> > {
>> > - if (!_M_spec._M_debug)
>> > - __throw_format_error("format error: invalid weekday");
>> > -
>> > _CharT __buf[3];
>> > __out = __format::__write(std::move(__out),
>> > _S_str_d1(__buf,
>> > __wd.c_encoding()));
>> > @@ -1183,11 +1216,8 @@ namespace __format
>> > {
>> > // %b Locale's abbreviated month name.
>> > // %B Locale's full month name.
>> > - if (!__m.ok())
>> > + if (_M_spec._M_debug && !__m.ok())
>> > {
>> > - if (!_M_spec._M_debug)
>> > - __throw_format_error("format error: invalid month");
>> > -
>> > _CharT __buf[3];
>> > __out = __format::__write(std::move(__out),
>> > _S_str_d1(__buf, (unsigned)__m));
>> > @@ -1419,7 +1449,7 @@ namespace __format
>> > _OutIter
>> > _M_j(const _ChronoData<_CharT>& __t, _OutIter __out) const
>> > {
>> > - if (_M_spec._M_time_only)
>> > + if (!_M_spec._M_needs(_ChronoParts::_DayOfYear))
>> > {
>> > // Decimal number of days, without padding.
>> > auto __d = chrono::floor<chrono::days>(__t._M_hours).count();
>> > @@ -1811,7 +1841,7 @@ namespace __format
>> > using enum _ChronoParts;
>> >
>> > _ChronoSpec<_CharT> __res{};
>> > - __res._M_time_only = (__parts & _Date) == 0;
>> > + __res._M_time_point = (__parts & _DateTime) == _DateTime;
>> > __res._M_floating_point_rep =
>> > chrono::treat_as_floating_point_v<_Rep>;
>> > __res._M_custom_rep = !is_arithmetic_v<_Rep>;
>> > __res._M_prec = chrono::hh_mm_ss<_Duration>::fractional_width;
>> > diff --git
>> > a/libstdc++-v3/testsuite/std/time/format/pr117214_custom_timeput.cc
>> > b/libstdc++-v3/testsuite/std/time/format/pr117214_custom_timeput.cc
>> > new file mode 100644
>> > index 00000000000..03b94961096
>> > --- /dev/null
>> > +++ b/libstdc++-v3/testsuite/std/time/format/pr117214_custom_timeput.cc
>> > @@ -0,0 +1,37 @@
>> > +// { dg-do run { target c++20 } }
>> > +
>> > +#include <chrono>
>> > +#include <format>
>> > +#include <locale>
>> > +#include <testsuite_hooks.h>
>> > +
>> > +struct custom_time_put : std::time_put<char>
>> > +{
>> > + iter_type
>> > + do_put(iter_type out, std::ios_base& io, char_type fill, const tm* t,
>> > + char format, char modifier) const override
>> > + {
>> > + using Base = std::time_put<char>;
>> > +
>> > + switch (format) {
>> > + case 'a': case 'A': case 'b': case 'h': case 'B': case 'p':
>> > + *out++ = '[';
>> > + *out++ = format;
>> > + *out++ = ']';
>> > + }
>> > + return Base::do_put(out, io, fill, t, format, modifier);
>> > + }
>> > +};
>> > +
>> > +int main()
>> > +{
>> > + using namespace std::chrono;
>> > + std::locale loc(std::locale::classic(), new custom_time_put);
>> > +#define test(t, fmt, exp) VERIFY( std::format(loc, fmt, t) == exp )
>> > + test(Monday, "{:L%a}", "[a]Mon");
>> > + test(Monday, "{:L%A}", "[A]Monday");
>> > + test(January, "{:L%b}", "[b]Jan");
>> > + test(January, "{:L%h}", "[h]Jan");
>> > + test(January, "{:L%B}", "[B]January");
>> > + test(1h, "{:L%p}", "[p]AM");
>> > +}
>> > --
>> > 2.49.0
>> >
>>
