Fix time utils time_t 64 bits in windows #134
Conversation
Signed-off-by: Eugenio Collado <[email protected]>
EugenioCollado
had a problem deploying
to
codecov
GitHub Actions
Failure
Signed-off-by: Eugenio Collado <[email protected]>
EugenioCollado
temporarily deployed
to
codecov
GitHub Actions
Inactive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #134 +/- ##
==========================================
- Coverage 65.97% 65.85% -0.13%
==========================================
Files 64 64
Lines 1699 1693 -6
Branches 484 481 -3
==========================================
- Hits 1121 1115 -6
Misses 278 278
Partials 300 300 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Eugenio Collado <[email protected]>
| @@ -32,7 +32,8 @@ using Duration_ms = uint32_t; | |||
| /** | |||
| * Type used to represent time points | |||
| */ | |||
| using Timestamp = std::chrono::time_point<std::chrono::system_clock>; | |||
| using Timeclock = std::chrono::system_clock; | |||
| using Timestamp = std::chrono::time_point<Timeclock>; | |||
There was a problem hiding this comment.
Please comment properly.
| // future time | ||
| { | ||
| Timestamp future_time = date_to_timestamp(2233u, 5u, 22u); | ||
| Timestamp future_time = date_to_timestamp(2033u, 5u, 22u); |
| } | ||
|
|
||
| Timestamp the_end_of_time() noexcept | ||
| { | ||
| return std::chrono::time_point<std::chrono::system_clock>::max(); | ||
| #if _EPROSIMA_WINDOWS_PLATFORM // In Windows std::gmtime does not support negative values nor values greater than 2^32 |
There was a problem hiding this comment.
I don't think we should be constraining precision due to limitations in a method we are only using for time to calendar conversions. I believe we should address the limitation in that method only, hoping the fix the problem in the future or we might as well change the implementation to not use that problematic method.
| } | ||
|
|
||
| Timestamp the_end_of_time() noexcept | ||
| { | ||
| return std::chrono::time_point<std::chrono::system_clock>::max(); | ||
| #if _EPROSIMA_WINDOWS_PLATFORM // In Windows std::gmtime does not support negative values nor values greater than 2^32 | ||
| return Timeclock::from_time_t(std::numeric_limits<long>::max()); |
There was a problem hiding this comment.
| return Timeclock::from_time_t(std::numeric_limits<long>::max()); | |
| return Timeclock::from_time_t(std::numeric_limits<int32_t>::max()); |
| } | ||
|
|
||
| Timestamp the_beginning_of_time() noexcept | ||
| { | ||
| return std::chrono::time_point<std::chrono::system_clock>::min(); | ||
| #if _EPROSIMA_WINDOWS_PLATFORM // In Windows std::gmtime does not support negative values nor values greater than 2^32 | ||
| return Timeclock::from_time_t(0); |
There was a problem hiding this comment.
Same as above. Apparently, at least in the Windows machine I have been testing, beginning of time actually is "1969-12-31_12-00-00", but I agree better to cut at 0. Mind blowing...
| #if _EPROSIMA_WINDOWS_PLATFORM // In Windows std::gmtime does not support negative values nor values greater than 2^32 | ||
| time_t max_value = std::numeric_limits<long>::max(); | ||
| duration_seconds = std::max((time_t) 0, std::min(max_value, duration_seconds)); | ||
| #endif // if _EPROSIMA_WINDOWS_PLATFORM |
There was a problem hiding this comment.
That's exactly what I meant. However perhaps we should print a warning if clamping has been done.
There was a problem hiding this comment.
I think we should also be handling similar problems in string_to_timestamp and date_to_timestamp.
This PR addresses an issue where Timestamp::max() or Timestamp::min() may exceed the representable range of std::tm, as time_t can be a 64-bit long long int. Specifically, std::tm in Windows cannot express dates before 1970 or beyond 2038.
To resolve this, the PR truncates time_t when converting it to std::tm and adjusts Timestamp::max() and Timestamp::min() to fit within the accepted range on Windows (0 to 2^32).