On Tue, Oct 6, 2026 at 10:11 AM Tomasz Kaminski <[email protected]> wrote:
> > > 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. > And I think that the ability to pre-allocate/re-use allocation for parsing many values is more useful than preserving, input value of *abbrev. > > >> >> 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 >>>>> >>>>>
