Skip to content

Write the time conversions with the C++20 <chrono> calendar types - #5342

Draft
GitPaean wants to merge 1 commit into
OPM:masterfrom
GitPaean:chrono-time-conversions
Draft

Write the time conversions with the C++20 <chrono> calendar types#5342
GitPaean wants to merge 1 commit into
OPM:masterfrom
GitPaean:chrono-time-conversions

Conversation

@GitPaean

@GitPaean GitPaean commented Sep 4, 2026

Copy link
Copy Markdown
Member

TimeService converted between std::time_t and civil time in two ways that both go wrong outside a narrow window, and neither is specific to any platform:

  • portable_timegm() computed 24L * days, so the expression was 32-bit wherever long is 32 bits: every date outside roughly 1902-2038 overflowed, and a schedule reaching year 3000 came back as 1911.
  • The other direction was *std::gmtime(&t), dereferenced without a check. std::gmtime() returns nullptr for time points its C runtime refuses - MSVC's for everything before 1970 and after year 3000, both of which schedules legitimately reach - so InteHEAD and DoubHEAD crashed writing the first report step of such a schedule. It also hands back a pointer to a static buffer, so concurrent conversions overwrite each other's result.

Both directions are now written with the C++20 calendar types, which are the same algorithms standardised. Only the calendar types - sys_days, year_month_day, weekday - are used, never the time zone part of the library: nothing here needs tzdata, and nothing needs the libstdc++ 14 that shipped it. The calendar types are in every standard library OPM builds with (libstdc++ 11, libc++, MSVC 2019 16.10 and later).

  • portable_timegm() builds std::chrono::sys_days from year/month/1 and adds the day of the month, so a day beyond the month still counts on from its first (33 January is 2 February): the wrap-around mkdatetime() relies on to reject such input. The month is normalised into the year as before.
  • TimeService::portable_gmtime() replaces std::gmtime(), and sits beside portable_timegm() as its inverse: it takes the day count apart with year_month_day and weekday, fills every field std::gmtime() fills (DoubHEAD's day-of-year calculation reads tm_yday), and returns the std::tm by value. Its floor division splits time_t into day and second taking quotient and remainder first, since the shorter (t - 86399) / 86400 overflows at the smallest time_t. TimeStampUTC(time_t), InteHEAD, DoubHEAD and OutputStream go through it.
  • The accepted range is std::chrono::year's, -32767-01-01 to 32767-12-31, in both directions; outside it either function throws std::out_of_range, the bounds taken from itself and shared by both. portable_timegm() checks the finished instant, not only the year, since the day of the month or the time of day can carry a std::tm past the end (32767-12-32, or 24:00 on the last day) - what it returns, portable_gmtime() always takes back. No schedule is anywhere near either end, and a refusal is the defined behaviour in place of a crash or a silently wrong date. On Linux the old code accepted such a date and produced a wrong one; a deck that holds one now fails with the DATES record's location, since ScheduleDeck wraps the throw in an OpmInputError.
  • Every second count is a long long, in the bounds and in the arithmetic on the std::tm fields alike, so an absurd field ends in the refusal above rather than in an overflow on the way there. Not std::time_t: the bounds are near 1e12, and a constant expression that overflows is ill-formed rather than merely wrong, so a 32-bit time_t would not compile. Leaving the arithmetic at the width of long is what the old conversion got wrong.
  • That fixes the arithmetic, but a std::time_t is still what both conversions take and return, and a narrow one would truncate every date the bounds admit. A static_assert requires the 64 bits instead, since a schedule runs past 2038 as a matter of course. This is deliberate: it refuses to build on a platform with a 32-bit time_t, which none of OPM's targets is, rather than let dates go wrong silently there.
  • The hand-written days_from_civil() is gone; nothing else used it.

tests/test_TimeService.cpp covers both directions: dates on both sides of the epoch and past year 3000, tm_yday and tm_wday against known dates, year 1, a round trip through TimeStampUTC, both ends of the calendar with the first instant past each refused, a std::tm carried past either end by its year, its day of the month or its time of day refused in the other direction, absurd tm fields refused, and the ends of the 64-bit range. It also covers mkdate() still rejecting 30 February, 33 January and month 13, which nothing else in the suite did: that rejection works only because portable_timegm() lets the day of the month carry instead of normalising it away or refusing it, so a later rewrite of the conversion could drop it silently.

@GitPaean GitPaean added the manual:irrelevant This PR is a minor fix and should not appear in the manual label Sep 4, 2026
@GitPaean
GitPaean force-pushed the chrono-time-conversions branch from 72e6c19 to f08612b Compare September 4, 2026 09:49
TimeService converted between std::time_t and civil time in two ways
that both go wrong outside a narrow window, and neither is specific to
any platform:

- portable_timegm() computed 24L * days, so the expression was 32-bit
  wherever long is 32 bits: every date outside roughly 1902-2038
  overflowed, and a schedule reaching year 3000 came back as 1911.
- The other direction was *std::gmtime(&t), dereferenced without a
  check. std::gmtime() returns nullptr for time points its C runtime
  refuses - MSVC's for everything before 1970 and after year 3000, both
  of which schedules legitimately reach - so InteHEAD and DoubHEAD
  crashed writing the first report step of such a schedule. It also
  hands back a pointer to a static buffer, so concurrent conversions
  overwrite each other's result.

Both directions are now written with the C++20 <chrono> calendar
types, which are the same algorithms standardised. Only the calendar
types - sys_days, year_month_day, weekday - are used, never the time
zone part of the library: nothing here needs tzdata, and nothing needs
the libstdc++ 14 that shipped it. The calendar types are in every
standard library OPM builds with (libstdc++ 11, libc++, MSVC 2019
16.10 and later).

- portable_timegm() builds std::chrono::sys_days from year/month/1 and
  adds the day of the month, so a day beyond the month still counts on
  from its first (33 January is 2 February): the wrap-around
  mkdatetime() relies on to reject such input. The month is normalised
  into the year as before.
- TimeService::portable_gmtime() replaces std::gmtime(), and sits
  beside portable_timegm() as its inverse: it takes the day count apart
  with year_month_day and weekday, fills every field std::gmtime()
  fills (DoubHEAD's day-of-year calculation reads tm_yday), and returns
  the std::tm by value. Its floor division splits time_t into day and
  second taking quotient and remainder first, since the shorter
  (t - 86399) / 86400 overflows at the smallest time_t.
  TimeStampUTC(time_t), InteHEAD, DoubHEAD and OutputStream go through
  it.
- The accepted range is std::chrono::year's, -32767-01-01 to
  32767-12-31, in both directions; outside it either function throws
  std::out_of_range, the bounds taken from <chrono> itself and shared
  by both. portable_timegm() checks the finished instant, not only the
  year, since the day of the month or the time of day can carry a
  std::tm past the end (32767-12-32, or 24:00 on the last day) - what
  it returns, portable_gmtime() always takes back. No schedule is
  anywhere near either end, and a refusal is the defined behaviour in
  place of a crash or a silently wrong date. On Linux the old code
  accepted such a date and produced a wrong one; a deck that holds one
  now fails with the DATES record's location, since ScheduleDeck wraps
  the throw in an OpmInputError.
- Every second count is a long long, in the bounds and in the
  arithmetic on the std::tm fields alike, so an absurd field ends in
  the refusal above rather than in an overflow on the way there. Not
  std::time_t: the bounds are near 1e12, and a constant expression that
  overflows is ill-formed rather than merely wrong, so a 32-bit time_t
  would not compile. Leaving the arithmetic at the width of long is
  what the old conversion got wrong.
- That fixes the arithmetic, but a std::time_t is still what both
  conversions take and return, and a narrow one would truncate every
  date the bounds admit. A static_assert requires the 64 bits instead,
  since a schedule runs past 2038 as a matter of course. This is
  deliberate: it refuses to build on a platform with a 32-bit time_t,
  which none of OPM's targets is, rather than let dates go wrong
  silently there.
- The hand-written days_from_civil() is gone; nothing else used it.

tests/test_TimeService.cpp covers both directions: dates on both sides
of the epoch and past year 3000, tm_yday and tm_wday against known
dates, year 1, a round trip through TimeStampUTC, both ends of the
calendar with the first instant past each refused, a std::tm carried
past either end by its year, its day of the month or its time of day
refused in the other direction, absurd tm fields refused, and the ends
of the 64-bit range. It also covers mkdate() still rejecting 30
February, 33 January and month 13, which nothing else in the suite
did: that rejection works only because portable_timegm() lets the day
of the month carry instead of normalising it away or refusing it, so a
later rewrite of the conversion could drop it silently.
@GitPaean

GitPaean commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

jenkins build this please

@bska bska left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't do this.

It's much better to actually use <chrono>'s calendar facilities throughout the code base than to patch this narrow calculation.

@GitPaean

GitPaean commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Please don't do this.

It's much better to actually use <chrono>'s calendar facilities throughout the code base than to patch this narrow calculation.

Thanks for the quick comments. I was not sure about the approach.

@GitPaean
GitPaean marked this pull request as draft September 4, 2026 10:21
@bska

bska commented Sep 4, 2026

Copy link
Copy Markdown
Member

I was not sure about the approach.

Right, no worries. For what it's worth, I've sort of started to scope out the work needed to replace all uses of time_t/gmtime()/localtime() with the proper <chrono> alternatives like std::chrono::sys_time<>. If you'd like to be a part of that project, then let me know.

@GitPaean

GitPaean commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

I was not sure about the approach.

Right, no worries. For what it's worth, I've sort of started to scope out the work needed to replace all uses of time_t/gmtime()/localtime() with the proper <chrono> alternatives like std::chrono::sys_time<>. If you'd like to be a part of that project, then let me know.

Yeah. If you have a branch already, I'd be glad to look at it. Otherwise I can revise based on your comments, so you can tell me whether it's the direction you want. You can review as it goes, or it's entirely fine if you'd rather do it yourself.

Mainly I'd like to get this done so I can stop carrying it in a local branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual:irrelevant This PR is a minor fix and should not appear in the manual

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants