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?
>
>
> > 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
> >
>
>
