fgerlits commented on a change in pull request #840:
URL: https://github.com/apache/nifi-minifi-cpp/pull/840#discussion_r455927688
##########
File path: libminifi/include/utils/TimeUtil.h
##########
@@ -108,23 +144,9 @@ inline int64_t parseDateTimeStr(const std::string &str) {
timeinfo.tm_hour = hours;
timeinfo.tm_min = minutes;
timeinfo.tm_sec = seconds;
+ timeinfo.tm_isdst = 0;
- /* Get local timezone offset */
- time_t utc = time(nullptr);
- struct tm now_tm = *gmtime(&utc); // NOLINT
- now_tm.tm_isdst = 0;
- time_t local = mktime(&now_tm);
- if (local == -1) {
- return -1;
- }
- int64_t timezone_offset = utc - local;
-
- /* Convert parsed date */
- time_t time = mktime(&timeinfo);
- if (time == -1) {
- return -1;
- }
- return time + timezone_offset;
+ return static_cast<int64_t>(mkgmtime(&timeinfo));
Review comment:
I didn't know about `timegm`; we could use that on Linux, but we'd still
need the rest of the code for non-Linux non-Windows compiles (I guess that's
why you said don't use it).
Leap seconds occur in UTC, but not in Unix time. This means that arguably
`parseDateTimeStr("2016-12-31T23:59:60Z")` should return 1483228799 (ie. the
same as `2016-12-31T23:59:59Z`) instead of 1483228800 (ie. the same as
`2017-01-01T00:00:00Z`). But mktime doesn't do this, either, so the old
`parseDateTimeStr` before this pull request also returned 1483228799.
I would not bring in a new library just to fix a bug that only affects dates
on 1 January 1970. If you hate this change too much, my preferred second best
solution would be to simply leave parseDateTimeStr as it is, and document this
limitation ("doesn't work for some times on 1 January 1970 in some time zones
on some architectures") in the unit test.
I understand your point, and agree that MiNiFi code should not contain an
implementation of the Gregorian calendar, as it is not its responsibility, but
I think having it temporarily until we can upgrade to C++20 is not terrible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
