[Bug libstdc++/114244] New: Need to use round when parsing fractional seconds

[Bug libstdc++/114244] New: Need to use round when parsing fractional seconds

[howard.hinnant at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114244

            Bug ID: 114244
           Summary: Need to use round when parsing fractional seconds
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: howard.hinnant at gmail dot com
  Target Milestone: ---

I'm not sure if this is a bug or just QOI.  But consider:

#include <chrono>
#include <string>
#include <iostream>


using time_point_t = std::chrono::sys_time<std::chrono::milliseconds>;

time_point_t decrypt(std::string s) {
    time_point_t tp;
    std::istringstream in(s);
    in >> parse("%Y%m%d-%T", tp);
    return tp;
}

int main() {
    std::cout << decrypt("20240304-13:00:00.002");
}

I expect the output to be:

2024-03-04 13:00:00.002

but it is:

2024-03-04 13:00:00.001

I believe the issue is the use of duration_cast<milliseconds> or
time_point_cast<milliseconds> somewhere under the parse (not sure exactly
where).  Since 0.002 is not exactly representable in floating point, we're
getting a truncate towards zero because this parses as just barely under 0.002.

I highly recommend the use of round<Duration> when converting floating point
based durations and time_points to integral based.

[Bug libstdc++/114244] Need to use round when parsing fractional seconds

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114244

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
   Last reconfirmed|                            |2024-03-05

[Bug libstdc++/114244] Need to use round when parsing fractional seconds

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114244

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yup, the seconds part "00.002" is parsed using std::numpunct (in order to
handle the locale's decimal point) and then converted to milliseconds using
duration_cast:

                          auto& __ng = use_facet<num_get<_CharT>>(__loc);
                          long double __val;
                          ios_base::iostate __err2{};
                          __ng.get(__buf, {}, __buf, __err2, __val);
                          if (__is_failed(__err2)) [[unlikely]]
                            __err |= __err2;
                          else
                            {
                              duration<long double> __fs(__val);
                              __s = duration_cast<_Duration>(__fs);
                            }


We also use duration_cast when parsing %c and %X using std::time_get, but
that's an integer conversion there (but the duration_cast should have been
qualified to prevent ADL):

                          __h = hours(__tm.tm_hour);
                          __min = minutes(__tm.tm_min);
                          __s = duration_cast<_Duration>(seconds(__tm.tm_sec));

and another duration_cast in chrono::from_stream for durations. That one could
be used with either integral or floating-point reps.

Do we want to parse "00:00:31" as minutes(1)? I don't think we do, so only the
first case where converting long double to milliseconds should be rounded?

[Bug libstdc++/114244] Need to use round when parsing fractional seconds

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114244

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #1)
> and another duration_cast in chrono::from_stream for durations. That one
> could be used with either integral or floating-point reps.

Ah, but we're converting from common_type_t<Duration, seconds> to Duration, so
either it's float-to-float or integer-to-integer, and duration_cast is OK if
we're fine with truncating.

[Bug libstdc++/114244] Need to use round when parsing fractional seconds

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114244

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #1)
> Yup, the seconds part "00.002" is parsed using std::numpunct (in order to

Oops, std::num_get obviously.

> handle the locale's decimal point) and then converted to milliseconds using
> duration_cast:
> 
> 			  auto& __ng = use_facet<num_get<_CharT>>(__loc);
> 			  long double __val;
> 			  ios_base::iostate __err2{};
> 			  __ng.get(__buf, {}, __buf, __err2, __val);
> 			  if (__is_failed(__err2)) [[unlikely]]
> 			    __err |= __err2;
> 			  else
> 			    {
> 			      duration<long double> __fs(__val);
> 			      __s = duration_cast<_Duration>(__fs);
> 			    }

As an unrelated optimization, when the locale is "C" we could use
std::from_chars or std::stod instead of std::num_get.

[Bug libstdc++/114244] Need to use round when parsing fractional seconds

[howard.hinnant at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114244

--- Comment #4 from Howard Hinnant <howard.hinnant at gmail dot com> ---
Not positive (because I don't know your code that well), but I think:

              __s = round<_Duration>(__fs);

will do it.

[Bug libstdc++/114244] Need to use round when parsing fractional seconds

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114244

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yup, that's what I have in my local tree now.

[Bug libstdc++/114244] Need to use round when parsing fractional seconds

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114244

--- Comment #6 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:5f9d7a5b6cf64639274e63051caf70fbc8418ea2

commit r14-9376-g5f9d7a5b6cf64639274e63051caf70fbc8418ea2
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Mar 7 13:15:41 2024 +0000

    libstdc++: Fix parsing of fractional seconds [PR114244]

    When converting a chrono::duration<long double> to a result type with an
    integer representation we should use chrono::round<_Duration> so that we
    don't truncate towards zero. Rounding ensures that e.g. 0.001999s
    becomes 2ms not 1ms.

    We can also remove some redundant uses of chrono::duration_cast to
    convert from seconds to _Duration, because the _Parser class template
    requires _Duration type to be able to represent seconds without loss of
    precision.

    This also fixes a bug where no fractional part would be parsed for
    chrono::duration<long double> because its period is ratio<1>. We should
    also consider treat_as_floating_point<rep> when deciding whether to skip
    reading a fractional part.

    libstdc++-v3/ChangeLog:

            PR libstdc++/114244
            * include/bits/chrono_io.h (_Parser::operator()): Remove
            redundant uses of duration_cast. Use chrono::round to convert
            long double value to durations with integer representations.
            Check represenation type when deciding whether to skip parsing
            fractional seconds.
            * testsuite/20_util/duration/114244.cc: New test.
            * testsuite/20_util/duration/io.cc: Check that a floating-point
            duration with ratio<1> precision can be parsed.

[Bug libstdc++/114244] Need to use round when parsing fractional seconds

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114244

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.3

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed on trunk so far.

[Bug libstdc++/114244] Need to use round when parsing fractional seconds

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114244

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.3                        |13.4

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 13.3 is being released, retargeting bugs to GCC 13.4.

[Bug libstdc++/114244] Need to use round when parsing fractional seconds

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114244

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|13.4                        |14.0

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #7)
> Fixed on trunk so far.

I'm not sure why I set the target milestone to 13.3, as chrono::parse isn't
present in the gcc-13 branch at all. So this bug only affected gcc-14 and was
fixed in time for the 14.1 release.