Nobody had any comments, so I've pushed it to trunk now.
On Thu, 26 Sept 2024 at 13:57, Jonathan Wakely <jwak...@redhat.com> wrote:
>
> Does this rounding heuristic seem reasonable? I have discussed it with
> Howard and he agreed that rounding "2024-09-22 18:34:56" up to the next
> day, 2024-09-23, could be surprising. But he convinced me that using
> chrono::round was correct in general (certainly better than what we have
> now using chrono::time_point_cast). A period like ratio<1,60> (e.g.
> a 60Hz refresh rate) can't be represented exactly with decimal digits,
> so we want to round from the parsed value to the closest representation
> in the result type. Truncating is strictly inferior when we're dealing
> with such periods.
>
> The hybrid rounding approach here tries to Do The Right Thing.
>
> MSVC seems to avoid this problem by just refusing to parse the date time
> "2024-09-22 18:34:56" into a result type of sys_days, presumably because
> the parsed value cannot be represented in the result type. I can see
> some justification for that, but I think rounding is more useful than
> failing to parse.
>
> Tested x86_64-linux.
>
> -- >8 --
>
> I noticed that chrono::parse was using duration_cast and time_point_cast
> to convert the parsed value to the result. Those functions truncate
> towards zero, which is not generally what you want. Especially for
> negative times before the epoch, where truncating towards zero rounds
> "up" towards the next duration/time_point. Using chrono::round is
> typically better, as that rounds to nearest.
>
> However, while testing the fix I realised that rounding to the nearest
> can give surprising results in some cases. For example if we parse a
> chrono::sys_days using chrono::parse("F %T", "2024-09-22 18:34:56", tp)
> then we will round up to the next day, i.e. sys_days(2024y/09/23). That
> seems surprising, and I think 2024-09-22 is what most users would
> expect.
>
> This change attempts to provide a hybrid rounding heuristic where we use
> chrono::round for the general case, but when the result has a period
> that is one of minutes, hours, days, weeks, or years then we truncate
> towards negative infinity using chrono::floor. This means that we
> truncate "2024-09-22 18:34:56" to the start of the current
> minute/hour/day/week/year, instead of rounding up to 2024-09-23, or to
> 18:35, or 17:00. For a period of months chrono::round is used, because
> the months duration is defined as a twelfth of a year, which is not
> actually the length of any calendar month. We don't want to truncate to
> a whole number of "months" if that can actually go from e.g. 2023-03-01
> to 2023-01-31, because February is shorter than chrono::months(1).
>
> libstdc++-v3/ChangeLog:
>
> * include/bits/chrono_io.h (__detail::__use_floor): New
> function.
> (__detail::__round): New function.
> (from_stream): Use __detail::__round.
> * testsuite/std/time/clock/file/io.cc: Check for expected
> rounding in parse.
> * testsuite/std/time/clock/gps/io.cc: Likewise.
> ---
> libstdc++-v3/include/bits/chrono_io.h | 64 +++++++++++++++++--
> .../testsuite/std/time/clock/file/io.cc | 21 +++++-
> .../testsuite/std/time/clock/gps/io.cc | 20 ++++++
> 3 files changed, 97 insertions(+), 8 deletions(-)
>
> diff --git a/libstdc++-v3/include/bits/chrono_io.h
> b/libstdc++-v3/include/bits/chrono_io.h
> index 1e34c82b532..362bb5aa9e9 100644
> --- a/libstdc++-v3/include/bits/chrono_io.h
> +++ b/libstdc++-v3/include/bits/chrono_io.h
> @@ -2407,6 +2407,56 @@ namespace __detail
> template<typename _Duration>
> using _Parser_t = _Parser<common_type_t<_Duration, seconds>>;
>
> + template<typename _Duration>
> + consteval bool
> + __use_floor()
> + {
> + if constexpr (_Duration::period::den == 1)
> + {
> + switch (_Duration::period::num)
> + {
> + case minutes::period::num:
> + case hours::period::num:
> + case days::period::num:
> + case weeks::period::num:
> + case years::period::num:
> + return true;
> + }
> + }
> + return false;
> + }
> +
> + // A "do the right thing" rounding function for duration and time_point
> + // values extracted by from_stream. When treat_as_floating_point is true
> + // we don't want to do anything, just a straightforward conversion.
> + // When the destination type has a period of minutes, hours, days, weeks,
> + // or years, we use chrono::floor to truncate towards negative infinity.
> + // This ensures that an extracted timestamp such as 2024-09-05 13:00:00
> + // will produce 2024-09-05 when rounded to days, rather than rounding up
> + // to 2024-09-06 (a different day).
> + // Otherwise, use chrono::round to get the nearest value representable
> + // in the destination type.
> + template<typename _ToDur, typename _Tp>
> + constexpr auto
> + __round(const _Tp& __t)
> + {
> + if constexpr (__is_duration_v<_Tp>)
> + {
> + if constexpr (treat_as_floating_point_v<typename _Tp::rep>)
> + return chrono::duration_cast<_ToDur>(__t);
> + else if constexpr (__detail::__use_floor<_ToDur>())
> + return chrono::floor<_ToDur>(__t);
> + else
> + return chrono::round<_ToDur>(__t);
> + }
> + else
> + {
> + static_assert(__is_time_point_v<_Tp>);
> + using _Tpt = time_point<typename _Tp::clock, _ToDur>;
> + return _Tpt(__detail::__round<_ToDur>(__t.time_since_epoch()));
> + }
> + }
> +
> } // namespace __detail
> /// @endcond
>
> @@ -2421,7 +2471,7 @@ namespace __detail
> auto __need = __format::_ChronoParts::_TimeOfDay;
> __detail::_Parser_t<duration<_Rep, _Period>> __p(__need);
> if (__p(__is, __fmt, __abbrev, __offset))
> - __d = chrono::duration_cast<duration<_Rep, _Period>>(__p._M_time);
> + __d = __detail::__round<duration<_Rep, _Period>>(__p._M_time);
> return __is;
> }
>
> @@ -2882,7 +2932,7 @@ namespace __detail
> else
> {
> auto __st = __p._M_sys_days + __p._M_time - *__offset;
> - __tp = chrono::time_point_cast<_Duration>(__st);
> + __tp = __detail::__round<_Duration>(__st);
> }
> }
> return __is;
> @@ -2918,7 +2968,7 @@ namespace __detail
> // "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 = chrono::time_point_cast<_Duration>(__ut);
> + __tp = __detail::__round<_Duration>(__ut);
> }
> return __is;
> }
> @@ -2956,7 +3006,7 @@ namespace __detail
> constexpr sys_days __epoch(-days(4383)); // 1958y/1/1
> auto __d = __p._M_sys_days - __epoch + __p._M_time - *__offset;
> tai_time<common_type_t<_Duration, seconds>> __tt(__d);
> - __tp = chrono::time_point_cast<_Duration>(__tt);
> + __tp = __detail::__round<_Duration>(__tt);
> }
> }
> return __is;
> @@ -2995,7 +3045,7 @@ namespace __detail
> constexpr sys_days __epoch(days(3657)); // 1980y/1/Sunday[1]
> auto __d = __p._M_sys_days - __epoch + __p._M_time - *__offset;
> gps_time<common_type_t<_Duration, seconds>> __gt(__d);
> - __tp = chrono::time_point_cast<_Duration>(__gt);
> + __tp = __detail::__round<_Duration>(__gt);
> }
> }
> return __is;
> @@ -3020,7 +3070,7 @@ namespace __detail
> {
> sys_time<_Duration> __st;
> if (chrono::from_stream(__is, __fmt, __st, __abbrev, __offset))
> - __tp = chrono::time_point_cast<_Duration>(file_clock::from_sys(__st));
> + __tp = __detail::__round<_Duration>(file_clock::from_sys(__st));
> return __is;
> }
>
> @@ -3049,7 +3099,7 @@ namespace __detail
> {
> days __d = __p._M_sys_days.time_since_epoch();
> auto __t = local_days(__d) + __p._M_time; // ignore offset
> - __tp = chrono::time_point_cast<_Duration>(__t);
> + __tp = __detail::__round<_Duration>(__t);
> }
> return __is;
> }
> diff --git a/libstdc++-v3/testsuite/std/time/clock/file/io.cc
> b/libstdc++-v3/testsuite/std/time/clock/file/io.cc
> index 9da5019ab78..27b4507236e 100644
> --- a/libstdc++-v3/testsuite/std/time/clock/file/io.cc
> +++ b/libstdc++-v3/testsuite/std/time/clock/file/io.cc
> @@ -67,8 +67,27 @@ test_parse()
> VERIFY( abbrev == "456" );
> VERIFY( offset == (1h + 23min) );
>
> - ss.str("");
> + // Test rounding
> ss.clear();
> + ss.str("2224-09-06 23");
> + file_time<days> d;
> + ss >> parse("%F %H", d); // Should be truncated to start of day, not
> rounded.
> + ss.str("");
> + ss << d;
> + VERIFY( ss.str() == "2224-09-06 00:00:00" );
> + ss.str("1969-12-31 23");
> + ss >> parse("%F %H", d); // Should be truncated to start of day, not
> rounded.
> + ss.str("");
> + ss << d;
> + VERIFY( ss.str() == "1969-12-31 00:00:00" );
> +
> + file_time<duration<long, std::ratio<10>>> ds; // decaseconds
> + ss.str("2224-09-06 15:07:06");
> + ss >> parse("%F %T", ds); // Should be rounded to nearest decasecond.
> + ss << ds;
> + VERIFY( ss.str() == "2224-09-06 15:07:10" );
> +
> + ss.str("");
> ss << file_time<seconds>{};
> VERIFY( ss >> parse("%F %T", tp) );
> VERIFY( tp.time_since_epoch() == 0s );
> diff --git a/libstdc++-v3/testsuite/std/time/clock/gps/io.cc
> b/libstdc++-v3/testsuite/std/time/clock/gps/io.cc
> index c012520080a..647ed021b24 100644
> --- a/libstdc++-v3/testsuite/std/time/clock/gps/io.cc
> +++ b/libstdc++-v3/testsuite/std/time/clock/gps/io.cc
> @@ -75,6 +75,26 @@ test_parse()
> VERIFY( abbrev == "GPS" );
> VERIFY( offset == -(12h + 34min) );
>
> + // Test rounding
> + ss.clear();
> + ss.str("2224-09-06 23");
> + gps_time<days> d;
> + ss >> parse("%F %H", d); // Should be truncated to start of day, not
> rounded.
> + ss.str("");
> + ss << d;
> + VERIFY( ss.str() == "2224-09-06 00:00:00" );
> + ss.str("1969-12-31 23");
> + ss >> parse("%F %H", d); // Should be truncated to start of day, not
> rounded.
> + ss.str("");
> + ss << d;
> + VERIFY( ss.str() == "1969-12-31 00:00:00" );
> +
> + gps_time<duration<long, std::ratio<10>>> ds; // decaseconds
> + ss.str("2224-09-06 15:07:06");
> + ss >> parse("%F %T", ds); // Should be rounded to nearest decasecond.
> + ss << ds;
> + VERIFY( ss.str() == "2224-09-06 15:07:10" );
> +
> ss.str("");
> ss << gps_seconds{};
> VERIFY( ss >> parse("%F %T", tp) );
> --
> 2.46.1
>
