HowardHinnant / date

A date and time library based on the C++11/14/17 <chrono> header
Other
3.11k stars 673 forks source link

%I formatiing not displaying hours correctly #660

Open anthonylouisbsb opened 3 years ago

anthonylouisbsb commented 3 years ago

I have a problem to parse dates using the %I format option to timestamps. Check the following code:

  std::istringstream in("1986-12-01 10:01:01");
  std::chrono::time_point<std::chrono::system_clock, std::chrono::milliseconds> result;

  in >> date::parse("%Y-%m-%d %I:%M:%S", result);

  if (in.fail()) {
    return false;
  }

  auto date = date::floor<arrow_vendored::date::days>(result);
  auto ymd = date::year_month_day(date);

  // ignore the time part
  date::sys_seconds secs = date::sys_days(ymd.year() / ymd.month() / ymd.day());

  int64_t subseconds = 0;
  if (!ignore_time_in_day) {
    auto time = date::hh_mm_ss<std::chrono::milliseconds>(
                                   date::floor<std::chrono::milliseconds>(result - date));

    secs += (std::chrono::hours(time.hours().count()) +
             std::chrono::minutes(time.minutes().count()) +
             std::chrono::seconds(time.seconds().count()));

    subseconds = static_cast<int64_t>(time.subseconds().count());
  }

  *out = util::CastSecondsToUnit(response_unit, secs.time_since_epoch().count());

  if (response_unit != SECOND) {
    *out += subseconds;
  }

When I display the output variable out, considering that the ignore_time_in_day variable is false and the response_unit variable is different from SECOND, the result is 533779261000 that corresponds to the 1986-12-01 00:01:01 datetime, not the datetime defined by the user.

If I change the %Iformatter option to the 24h clock option %H, the method works fine.

I am doing something wrong in my code or there is a bug with the format option for 12h clocks?

HowardHinnant commented 3 years ago

This looks like a bug in date.h. I should set failbit if %I is used without %p. Otherwise 10:01:01 is ambiguous. Is that AM or PM?

I've just pushed a fix for this to master.

anthonylouisbsb commented 3 years ago

@HowardHinnant To keep a similar behavior with the native strptime function, I think the %I flag should consider as AM If the user does not define a value for the %p flag.

HowardHinnant commented 3 years ago

Thanks for the link to the linux docs. I looked there, and at the posix strptime spec. In neither place am I seeing a default for am/pm. Maybe I'm just missing it. Could you point to the part of the spec that says this? Thanks.

anthonylouisbsb commented 3 years ago

@HowardHinnant I did not find anything in specs too, I was based on an empirical test using the gcc compiler in the Apache Arrow project.

gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0

But I think your suggestion is right, if the user not define if it is AM or PM is ambiguous to know how to parse the date.

If I find something that says how the POSIX specs define the behavior for that case, I will share it here.

Thanks for the quick response and for correcting the problem!

anthonylouisbsb commented 3 years ago

@HowardHinnant I added a Pull Request with some unit tests related to the change you made.