On Tue, Oct 6, 2026 at 10:03 AM Jonathan Wakely <[email protected]> wrote:
> > > On Tue, 6 Oct 2026, 08:23 Tomasz Kaminski, <[email protected]> wrote: > >> >> >> On Tue, Oct 6, 2026 at 8:54 AM Tomasz Kaminski <[email protected]> >> wrote: >> >>> >>> >>> On Mon, Oct 5, 2026 at 11:02 PM Jonathan Wakely <[email protected]> >>> wrote: >>> >>>> Formatting utc_time can produce a time ending with ":60" for a leap >>>> second, so we should also be able to read that back using chrono::parse. >>>> >>>> We should reject ":60" unless the time really is a leap second, so >>>> compare to the list of known leap seconds. Continue to reject ":60" for >>>> any time point except utc_time. >>>> >>>> We need to delay writing to the *abbrev and *offset variables until >>>> after we know if the parse is successful, so after handling the >>>> _M_is_leap_second flag. >>>> >>>> libstdc++-v3/ChangeLog: >>>> >>>> PR libstdc++/126543 >>>> * include/bits/chrono_io.h (from_stream): Delay writing abbrev >>>> and offset until successful or failed parse has been decided. >>>> For utc_time use chrono::get_leap_second_info to check for valid >>>> leap seconds. >>>> (_Parser::operator()) <R>: Do not accept "60" for minutes. >>>> <S>: Accept "60" for seconds. >>>> * testsuite/std/time/parse/114279.cc: Use a real leap second as >>>> the test input. Check parsing whole seconds as well as >>>> fractional seconds. Check that offset isn't written to for >>>> failed parse. >>>> * testsuite/std/time/parse/126543.cc: New test. >>>> --- >>>> >>>> Tested x86_64-linux. >>>> >>>> libstdc++-v3/include/bits/chrono_io.h | 74 ++++++++++++------- >>>> .../testsuite/std/time/parse/114279.cc | 33 +++++++-- >>>> .../testsuite/std/time/parse/126543.cc | 40 ++++++++++ >>>> 3 files changed, 112 insertions(+), 35 deletions(-) >>>> create mode 100644 libstdc++-v3/testsuite/std/time/parse/126543.cc >>>> >>>> diff --git a/libstdc++-v3/include/bits/chrono_io.h >>>> b/libstdc++-v3/include/bits/chrono_io.h >>>> index eec12357fdf7..0d6da0eb1bd7 100644 >>>> --- a/libstdc++-v3/include/bits/chrono_io.h >>>> +++ b/libstdc++-v3/include/bits/chrono_io.h >>>> @@ -3911,24 +3911,27 @@ namespace __detail >>>> basic_istream<_CharT, _Traits>& >>>> from_stream(basic_istream<_CharT, _Traits>& __is, const _CharT* >>>> __fmt, >>>> sys_time<_Duration>& __tp, >>>> - basic_string<_CharT, _Traits, _Alloc>* __abbrev = >>>> nullptr, >>>> - minutes* __offset = nullptr) >>>> + basic_string<_CharT, _Traits, _Alloc>* __abbrev_p = >>>> nullptr, >>>> + minutes* __offset_p = nullptr) >>>> { >>>> + basic_string<_CharT, _Traits, _Alloc> __abbr; >>>> >>> I do not like how we are tripple buffering the __abber now: >>> * one string provided by the user provided by >>> * one created here >>> * one created in operator() >>> >> After re-reading the standard, I do not think it requires that: >> > We need to delay writing to the *abbrev and *offset variables until >> after we know if the parse succeeded >> >> The pagraph https://eel.is/c++draft/time.clock.system#nonmembers-6 says: >> > If %Z is used and successfully parsed, that value will be assigned to * >> abbrev if abbrev is non-null. >> <https://eel.is/c++draft/time.clock.system#nonmembers-6.sentence-3> >> Which for me reads us, if %Z was successfully parsed, the abbrev is >> changed, not if the whole thing >> was successfully parsed. >> > > I don't agree with that interpretation. > https://eel.is/c++draft/time.parse#17.sentence-1 says to me that parsing > all succeeds or all fails, there is no partial success for one part and > failure for another. > The sentence says that failbit is set, it does not mention whether the input values are modified. That does not contradict my claim that the wording says that abbrev is modified as soon as %Z is successfully parsed, the failbit (indicating whole parsing failure), may still be set. > > I agree we should get that clarified. > We should still fix the original issues, as they seem to be independed. > > >> I think this distinction is important, as it could potentially allow no >> allocations here >> std::string abbrev; >> while (in) >> from_stream("%T %Z", time, &abbrev); >> The second and later iterations can use the memory allocated for abbrev. >> This is meaningful if we >> use named zones, like Europe/Warsaw, and allows preallocation before. >> >> But, even my interpretation of %Z seem insufficient, as we cannot parse >> directly to *abbrev, because >> if the stream fails when reading additional characters, we cannot roll >> that back. I think we should clarify >> that wording, and either make *abbrev unspecified in case if parsing of >> %Z fails, all change the wording >> to make the changes atomic (either all are modified or none), as we >> require buffering anyway. In this >> case I would use my _M_fill_tz suggestion. >> >> In light of above could you change the patch to just >> add __p._M_is_leap_second checks, without changes >> to handling of abbrev/offset. >> >> My suggestion in the _Parser, add following members and remove them >>> from operator(): >>> minutes __M_offset = __bad_min; >>> basic_string<_CharT, _Traits> __M_abbr; >>> >> >>> Remove the __abbrev and __offset parameter from the operator() and >>> instead simply set __tz_offset and __tz_abbr always. >>> >>> And finally add _M_fill_tz(basic_string<_CharT, _Traits, _Alloc>* >>> __abbrev, >>> basic_string<_CharT, _Traits, >>> _Alloc>* __offset): >>> to the Parser, that will doo if (abbrev && _M_abbrev.empty()) abbrev = >>> std::move(_M_abbev). >>> And call it only when we are successful. >>> >>> minutes __off{}; >>>> - if (!__offset) >>>> - __offset = &__off; >>>> >>> So all of the above goes away. >>> >>>> using __format::_ChronoParts; >>>> auto __need = _ChronoParts::_Year | _ChronoParts::_Month >>>> | _ChronoParts::_Day | _ChronoParts::_TimeOfDay; >>>> __detail::_Parser_t<_Duration> __p(__need); >>>> - if (__p(__is, __fmt, __abbrev, __offset)) >>>> + if (__p(__is, __fmt, __abbrev_p ? &__abbr : nullptr, &__off)) >>>> { >>>> if (__p._M_is_leap_second) >>>> __is.setstate(ios_base::failbit); >>>> else >>>> { >>>> - auto __st = __p._M_sys_days + __p._M_time - *__offset; >>>> + auto __st = __p._M_sys_days + __p._M_time - __off; >>>> __tp = __detail::__round<_Duration>(__st); >>>> >>> And we call __p._M_fill_tz(__offset, __abbrev) here. >>> >>>> + if (__offset_p) >>>> + *__offset_p = __off; >>>> + if (__abbrev_p) >>>> + *__abbrev_p = std::move(__abbr); >>>> } >>>> } >>>> return __is; >>>> @@ -3945,23 +3948,32 @@ namespace __detail >>>> inline basic_istream<_CharT, _Traits>& >>>> from_stream(basic_istream<_CharT, _Traits>& __is, const _CharT* >>>> __fmt, >>>> utc_time<_Duration>& __tp, >>>> - basic_string<_CharT, _Traits, _Alloc>* __abbrev = >>>> nullptr, >>>> - minutes* __offset = nullptr) >>>> + basic_string<_CharT, _Traits, _Alloc>* __abbrev_p = >>>> nullptr, >>>> + minutes* __offset_p = nullptr) >>>> { >>>> + basic_string<_CharT, _Traits, _Alloc> __abbr; >>>> minutes __off{}; >>>> - if (!__offset) >>>> - __offset = &__off; >>>> using __format::_ChronoParts; >>>> auto __need = _ChronoParts::_Year | _ChronoParts::_Month >>>> | _ChronoParts::_Day | _ChronoParts::_TimeOfDay; >>>> __detail::_Parser_t<_Duration> __p(__need); >>>> - if (__p(__is, __fmt, __abbrev, __offset)) >>>> + if (__p(__is, __fmt, __abbrev_p ? &__abbr : nullptr, &__off)) >>>> { >>>> // Converting to utc_time before adding _M_time is necessary >>>> for >>>> // "23:59:60" to correctly produce a time within a leap >>>> second. >>>> auto __ut = utc_clock::from_sys(__p._M_sys_days) + __p._M_time >>>> - - *__offset; >>>> - __tp = __detail::__round<_Duration>(__ut); >>>> + - __off; >>>> + if (__p._M_is_leap_second >>>> + && !chrono::get_leap_second_info(__ut).is_leap_second) >>>> + __is.setstate(ios_base::failbit); >>>> + else >>>> + { >>>> + __tp = __detail::__round<_Duration>(__ut); >>>> + if (__offset_p) >>>> + *__offset_p = __off; >>>> + if (__abbrev_p) >>>> + *__abbrev_p = std::move(__abbr); >>>> + } >>>> } >>>> return __is; >>>> } >>>> @@ -3977,26 +3989,29 @@ namespace __detail >>>> inline basic_istream<_CharT, _Traits>& >>>> from_stream(basic_istream<_CharT, _Traits>& __is, const _CharT* >>>> __fmt, >>>> tai_time<_Duration>& __tp, >>>> - basic_string<_CharT, _Traits, _Alloc>* __abbrev = >>>> nullptr, >>>> - minutes* __offset = nullptr) >>>> + basic_string<_CharT, _Traits, _Alloc>* __abbrev_p = >>>> nullptr, >>>> + minutes* __offset_p = nullptr) >>>> { >>>> + basic_string<_CharT, _Traits, _Alloc> __abbr; >>>> minutes __off{}; >>>> - if (!__offset) >>>> - __offset = &__off; >>>> using __format::_ChronoParts; >>>> auto __need = _ChronoParts::_Year | _ChronoParts::_Month >>>> | _ChronoParts::_Day | _ChronoParts::_TimeOfDay; >>>> __detail::_Parser_t<_Duration> __p(__need); >>>> - if (__p(__is, __fmt, __abbrev, __offset)) >>>> + if (__p(__is, __fmt, __abbrev_p ? &__abbr : nullptr, &__off)) >>>> { >>>> if (__p._M_is_leap_second) >>>> __is.setstate(ios_base::failbit); >>>> else >>>> { >>>> constexpr sys_days __epoch(-days(4383)); // 1958y/1/1 >>>> - auto __d = __p._M_sys_days - __epoch + __p._M_time - >>>> *__offset; >>>> + auto __d = __p._M_sys_days - __epoch + __p._M_time - >>>> __off; >>>> tai_time<common_type_t<_Duration, seconds>> __tt(__d); >>>> __tp = __detail::__round<_Duration>(__tt); >>>> + if (__offset_p) >>>> + *__offset_p = __off; >>>> + if (__abbrev_p) >>>> + *__abbrev_p = std::move(__abbr); >>>> } >>>> } >>>> return __is; >>>> @@ -4013,25 +4028,28 @@ namespace __detail >>>> inline basic_istream<_CharT, _Traits>& >>>> from_stream(basic_istream<_CharT, _Traits>& __is, const _CharT* >>>> __fmt, >>>> gps_time<_Duration>& __tp, >>>> - basic_string<_CharT, _Traits, _Alloc>* __abbrev = >>>> nullptr, >>>> - minutes* __offset = nullptr) >>>> + basic_string<_CharT, _Traits, _Alloc>* __abbrev_p = >>>> nullptr, >>>> + minutes* __offset_p = nullptr) >>>> { >>>> + basic_string<_CharT, _Traits, _Alloc> __abbr; >>>> minutes __off{}; >>>> - if (!__offset) >>>> - __offset = &__off; >>>> using __format::_ChronoParts; >>>> auto __need = _ChronoParts::_YearMonthDay | >>>> _ChronoParts::_TimeOfDay; >>>> __detail::_Parser_t<_Duration> __p(__need); >>>> - if (__p(__is, __fmt, __abbrev, __offset)) >>>> + if (__p(__is, __fmt, __abbrev_p ? &__abbr : nullptr, &__off)) >>>> { >>>> if (__p._M_is_leap_second) >>>> __is.setstate(ios_base::failbit); >>>> else >>>> { >>>> constexpr sys_days __epoch(days(3657)); // >>>> 1980y/1/Sunday[1] >>>> - auto __d = __p._M_sys_days - __epoch + __p._M_time - >>>> *__offset; >>>> + auto __d = __p._M_sys_days - __epoch + __p._M_time - >>>> __off; >>>> gps_time<common_type_t<_Duration, seconds>> __gt(__d); >>>> __tp = __detail::__round<_Duration>(__gt); >>>> + if (__offset_p) >>>> + *__offset_p = __off; >>>> + if (__abbrev_p) >>>> + *__abbrev_p = std::move(__abbr); >>>> } >>>> } >>>> return __is; >>>> @@ -4769,7 +4787,7 @@ namespace __detail >>>> __h = hours(__val); >>>> >>>> __val = __read_unsigned(2); >>>> - if (__val == -1 || __val > 60) [[unlikely]] >>>> + if (__val == -1 || __val > 59) [[unlikely]] >>>> if ((_M_need & _ChronoParts::_TimeOfDay) != 0) >>>> { >>>> __err |= ios_base::failbit; >>>> @@ -4802,10 +4820,12 @@ namespace __detail >>>> && !__is_floating) >>>> { >>>> auto __val = __read_unsigned(__num ? __num : 2); >>>> - if (0 <= __val && __val <= 59) [[likely]] >>>> + if (0 <= __val && __val <= 60) [[likely]] >>>> __s = seconds(__val); >>>> else >>>> { >>>> + // Not a valid second, but we only fail if >>>> we're >>>> + // parsing into a type that cares about that. >>>> if ((_M_need & _ChronoParts::_TimeOfDay) != 0) >>>> __err |= ios_base::failbit; >>>> break; >>>> diff --git a/libstdc++-v3/testsuite/std/time/parse/114279.cc >>>> b/libstdc++-v3/testsuite/std/time/parse/114279.cc >>>> index 67e5cba23bd8..ff9a1f18112a 100644 >>>> --- a/libstdc++-v3/testsuite/std/time/parse/114279.cc >>>> +++ b/libstdc++-v3/testsuite/std/time/parse/114279.cc >>>> @@ -4,14 +4,18 @@ >>>> #include <sstream> >>>> #include <testsuite_hooks.h> >>>> >>>> -template<class Clock> >>>> +template<class Clock, class Dur> >>>> void >>>> -test_leap_second_parsing() >>>> +do_test_leap_second_parsing() >>>> { >>>> - std::chrono::time_point<Clock, std::chrono::milliseconds> tp, tp2; >>>> + std::chrono::time_point<Clock, Dur> tp, tp2; >>>> >>>> - std::istringstream ss("20161231-23:59:60.05"); >>>> - ss >> std::chrono::parse("%Y%m%d-%T", tp); >>>> + std::string subs; >>>> + if constexpr (std::ratio_less_v<typename Dur::period, std::ratio<1, >>>> 1>>) >>>> + subs = ".05"; >>>> + >>>> + std::istringstream ss("20081231-23:59:60" + subs + " "); >>>> + ss >> std::chrono::parse("%Y%m%d-%T ", tp); >>>> >>>> if constexpr (std::is_same_v<Clock, std::chrono::local_t>) >>>> VERIFY( ss ); // We allow parsing "23:59:60" as local_time. >>>> @@ -28,20 +32,33 @@ test_leap_second_parsing() >>>> else >>>> VERIFY( !ss ); // Other clocks do not allow "HH:MM:60" >>>> >>>> + std::chrono::minutes offset(9999); >>>> ss.clear(); >>>> - ss.str("20161231-22:59:60.05 -0100"); // Same time at -1h offset. >>>> - ss >> std::chrono::parse("%Y%m%d-%T %z", tp2); >>>> + ss.str("20081231-22:59:60" + subs + " -0100"); // Same time at -1h >>>> offset. >>>> + ss >> std::chrono::parse("%Y%m%d-%T %z", tp2, offset); >>>> >>>> if constexpr (std::is_same_v<Clock, std::chrono::utc_clock>) >>>> { >>>> VERIFY( ss ); >>>> VERIFY( tp2 == tp ); >>>> + VERIFY( offset == std::chrono::minutes(-60) ); >>>> } >>>> else >>>> - VERIFY( !ss ); >>>> + { >>>> + VERIFY( !ss ); >>>> + VERIFY( offset == std::chrono::minutes(9999) ); // unchanged on >>>> fail >>>> + } >>>> } >>>> } >>>> >>>> +template<class Clock> >>>> +void >>>> +test_leap_second_parsing() >>>> +{ >>>> + do_test_leap_second_parsing<Clock, std::chrono::milliseconds>(); >>>> + do_test_leap_second_parsing<Clock, std::chrono::seconds>(); >>>> +} >>>> + >>>> int main() >>>> { >>>> test_leap_second_parsing<std::chrono::system_clock>(); >>>> diff --git a/libstdc++-v3/testsuite/std/time/parse/126543.cc >>>> b/libstdc++-v3/testsuite/std/time/parse/126543.cc >>>> new file mode 100644 >>>> index 000000000000..2d2f633b4b7a >>>> --- /dev/null >>>> +++ b/libstdc++-v3/testsuite/std/time/parse/126543.cc >>>> @@ -0,0 +1,40 @@ >>>> +// { dg-do run { target c++20 } } >>>> + >>>> +// Bug 126543 - chrono::parse should accept "60" seconds for a utc_time >>>> +// that is a valid leap second >>>> + >>>> +#include <chrono> >>>> +#include <sstream> >>>> +#include <testsuite_hooks.h> >>>> + >>>> +int main() >>>> +{ >>>> + std::istringstream in; >>>> + std::chrono::utc_seconds ut; >>>> + std::chrono::sys_seconds st; >>>> + >>>> + in.str("2016-12-31 24:59:59"); // invalid hour >>>> + in >> parse("%F %T", ut); >>>> + VERIFY(in.fail()); >>>> + in.clear(); >>>> + >>>> + in.str("2016-12-31 23:60:59"); // invalid minute >>>> + in >> parse("%F %T", ut); >>>> + VERIFY(in.fail()); >>>> + in.clear(); >>>> + >>>> + in.str("2016-12-31 23:59:60"); // valid leap second >>>> + in >> parse("%F %T", ut); >>>> + VERIFY(in.good()); >>>> + in.clear(); >>>> + >>>> + in.str("2016-12-31 23:58:60"); // invalid leap second >>>> + in >> parse("%F %T", ut); >>>> + VERIFY(in.fail()); >>>> + in.clear(); >>>> + >>>> + in.str("2016-12-31 23:59:60"); >>>> + in >> parse("%F %T", st); >>>> + VERIFY(in.fail()); // sys_time doesn't use leap seconds >>>> + in.clear(); >>>> +} >>>> -- >>>> 2.55.0 >>>> >>>>
