[Bug libstdc++/114240] New: sys_days not being parsed with only a date in the stream

[Bug libstdc++/114240] New: sys_days not being parsed with only a date in the stream

[howard.hinnant at gmail dot com]

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

            Bug ID: 114240
           Summary: sys_days not being parsed with only a date in the
                    stream
           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: ---

#include <chrono>
#include <iostream>
#include <sstream>

int main(int argc, char**argv){

    std::istringstream ssStart{"2024-01-01"};
    std::istringstream ssEnd("2024-02-01");
    ssStart.exceptions(std::ios::failbit);

    std::chrono::sys_days tStart, tEnd;
    ssStart >> std::chrono::parse("%Y-%m-%d ", tStart);
    ssEnd >> std::chrono::parse("%Y-%m-%d ", tEnd);

    auto tDays = tEnd-tStart;
    std::cout << ssStart.str() << "," << ssEnd.str() << "," << tDays.count() <<
std::endl;
}

Expected output:

2024-01-01,2024-02-01,31

Actual output:

terminate called after throwing an instance of 'std::__ios_failure'
  what():  basic_ios::clear: iostream error

I expected this function: http://eel.is/c++draft/time.clock.system.nonmembers#6
to successfully parse the days-precision time_point because there is sufficient
information in the stream to form a valid date.

Demo: https://wandbox.org/permlink/FrtMteIddnb4ogpZ

[Bug libstdc++/114240] sys_days not being parsed with only a date in the stream

[redi at gcc dot gnu.org]

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

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

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

[Bug libstdc++/114240] sys_days not being parsed with only a date in the stream

[redi at gcc dot gnu.org]

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

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I think the problem is that I just have some generic logic that assumes all
sys_time specializations are a date time, and so require both a date and a
time. But obviously for sys_days we only need a date.

  template<typename _CharT, typename _Traits, typename _Duration,
           typename _Alloc = allocator<_CharT>>
    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)
    {
      minutes __off{};
      if (!__offset)
        __offset = &__off;
      using __format::_ChronoParts;
      auto __need = _ChronoParts::_Year | _ChronoParts::_Month
                    | _ChronoParts::_Day | _ChronoParts::_TimeOfDay;

This should depend on _Duration::period.

[Bug libstdc++/114240] sys_days not being parsed with only a date in the stream

[howard.hinnant at gmail dot com]

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

--- Comment #2 from Howard Hinnant <howard.hinnant at gmail dot com> ---
In my date lib I just presumed 00:00:00 time of day when parsing time_points,
unless the parse produced another time of day.  Though I must admit that this
didn't come through in the spec.  So there is a little grey area here.

We're going to hit the same issue for other timepoints as well:  local_time,
utc_time, etc.

[Bug libstdc++/114240] sys_days not being parsed with only a date in the stream

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
So this would fix it:

--- a/libstdc++-v3/include/bits/chrono_io.h
+++ b/libstdc++-v3/include/bits/chrono_io.h
@@ -2826,7 +2826,9 @@ namespace __detail
        __offset = &__off;
       using __format::_ChronoParts;
       auto __need = _ChronoParts::_Year | _ChronoParts::_Month
-                   | _ChronoParts::_Day | _ChronoParts::_TimeOfDay;
+                   | _ChronoParts::_Day;
+      if constexpr (ratio_less_v<typename _Duration::period, days::period>)
+       __need |= _ChronoParts::_TimeOfDay;
       __detail::_Parser_t<_Duration> __p(__need);
       if (__p(__is, __fmt, __abbrev, __offset))
        {

A similar fix is needed for the from_stream overload for local_time, but
utc_time, gps_time and tai_time will be fixed by changing the sys_time
overload.

[Bug libstdc++/114240] sys_days not being parsed with only a date in the stream

[redi at gcc dot gnu.org]

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This has revealed another bug in some of the from_stream overloads, due to
Clock::from_sys / Clock::from_utc sometimes returning a higher precision value
than the input argument (due to using the common_type with seconds).

[Bug libstdc++/114240] sys_days not being parsed with only a date in the stream

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
>        using __format::_ChronoParts;
>        auto __need = _ChronoParts::_Year | _ChronoParts::_Month
> -                   | _ChronoParts::_Day | _ChronoParts::_TimeOfDay;
> +                   | _ChronoParts::_Day;
> +      if constexpr (ratio_less_v<typename _Duration::period, days::period>)
> +       __need |= _ChronoParts::_TimeOfDay;

This condition's not right.

If we have a period of days, but a floating-point rep, we probably do want to
parse and use a time if one is present in the input. "2024-03-07 00:05" can be
represented by sys_time<duration<double, days::period>> and is not the same
time as "2024-03-07".

And if we have a period like ratio<days::period::num+1, 1> then it's greater
than days, but not using the time of day in the result will lose accuracy.

I'm leaning towards Howard's approach of just assuming 00:00:00 if no time is
present, rather than making it depend on the period in any way.

[Bug libstdc++/114240] sys_days not being parsed with only a date in the stream

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Actually the standard does support Howard's intended behaviour:

"If the parse fails to decode a valid date, is.setstate(ios_base::failbit) is
called and tp is not modified."

It says "date", not "time point" or "UTC time" or anything like that.

[Bug libstdc++/114240] sys_days not being parsed with only a date in the stream

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 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:3e8ee03edd018eed43444755f601cdb9d5931654

commit r14-9406-g3e8ee03edd018eed43444755f601cdb9d5931654
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Mar 8 16:15:57 2024 +0000

    libstdc++: Do not require a time-of-day when parsing sys_days [PR114240]

    When parsing a std::chrono::sys_days (or a sys_time with an even longer
    period) we should not require a time-of-day to be present in the input,
    because we can't represent that in the result type anyway.

    Rather than trying to decide which specializations should require a
    time-of-date and which should not, follow the direction of Howard
    Hinnant's date library, which allows extracting a sys_time of any period
    from input that only contains a date, defaulting the time-of-day part to
    00:00:00. This seems consistent with the intent of the standard, which
    says it's an error "If the parse fails to decode a valid date" (i.e., it
    doesn't care about decoding a valid time, only a date).

    libstdc++-v3/ChangeLog:

            PR libstdc++/114240
            * include/bits/chrono_io.h (_Parser::operator()): Assume
            hours(0) for a time_point, so that a time is not required
            to be present.
            * testsuite/std/time/parse/114240.cc: New test.

[Bug libstdc++/114240] sys_days not being parsed with only a date in the stream

[redi at gcc dot gnu.org]

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

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

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

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This should be fixed now. Thanks for the report.