You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/06/22 10:05:53 UTC
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1352: MINIFICPP-1842 getTimeStr should use std::chrono
fgerlits commented on code in PR #1352:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1352#discussion_r903517228
##########
libminifi/test/unit/TimeUtilTests.cpp:
##########
@@ -21,79 +21,60 @@
using namespace std::literals::chrono_literals;
-namespace {
- constexpr int ONE_HOUR = 60 * 60;
- constexpr int ONE_DAY = 24 * ONE_HOUR;
-
- struct tm createTm(int year, int month, int day, int hour, int minute, int second, bool is_dst = false) {
- struct tm date_time;
- date_time.tm_year = year - 1900;
- date_time.tm_mon = month - 1;
- date_time.tm_mday = day;
- date_time.tm_hour = hour;
- date_time.tm_min = minute;
- date_time.tm_sec = second;
- date_time.tm_isdst = is_dst ? 1 : 0;
- return date_time;
- }
-
- void mkgmtimeTestHelper(time_t expected, int year, int month, int day, int hour, int minute, int second) {
- using org::apache::nifi::minifi::utils::timeutils::mkgmtime;
- struct tm date_time = createTm(year, month, day, hour, minute, second);
- REQUIRE(mkgmtime(&date_time) == expected);
- }
-} // namespace
+template <class TimePointType>
+void testParseDateTimeStr() {
+ using org::apache::nifi::minifi::utils::timeutils::parseDateTimeStr;
-TEST_CASE("mkgmtime() works correctly", "[mkgmtime]") {
- mkgmtimeTestHelper(0, 1970, 1, 1, 0, 0, 0);
- for (int hour = 0; hour < 24; ++hour) {
- mkgmtimeTestHelper((hour + 1) * ONE_HOUR - 1, 1970, 1, 1, hour, 59, 59);
- }
-
- mkgmtimeTestHelper(ONE_DAY, 1970, 1, 2, 0, 0, 0);
- mkgmtimeTestHelper(31 * ONE_DAY, 1970, 2, 1, 0, 0, 0);
- mkgmtimeTestHelper(365 * ONE_DAY, 1971, 1, 1, 0, 0, 0);
-
- mkgmtimeTestHelper(793929600, 1995, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(793929600 + ONE_DAY, 1995, 3, 1, 0, 0, 0);
- mkgmtimeTestHelper(825465600, 1996, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(825465600 + ONE_DAY, 1996, 2, 29, 0, 0, 0);
- mkgmtimeTestHelper(951696000, 2000, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(951696000 + ONE_DAY, 2000, 2, 29, 0, 0, 0);
- mkgmtimeTestHelper(4107456000, 2100, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(4107456000 + ONE_DAY, 2100, 3, 1, 0, 0, 0);
-
- mkgmtimeTestHelper(1513104856, 2017, 12, 12, 18, 54, 16);
- mkgmtimeTestHelper(1706655675, 2024, 1, 30, 23, 01, 15);
- mkgmtimeTestHelper(3710453630, 2087, 7, 31, 01, 33, 50);
+ CHECK(*parseDateTimeStr<TimePointType>("1970-01-01T00:00:00Z") == TimePointType{0s});
+ CHECK(*parseDateTimeStr<TimePointType>("1970-01-01T00:59:59Z") == TimePointType{1h - 1s});
+
+ CHECK(*parseDateTimeStr<TimePointType>("1970-01-02T00:00:00Z") == TimePointType{std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("1970-02-01T00:00:00Z") == TimePointType{31 * std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("1971-01-01T00:00:00Z") == TimePointType{365 * std::chrono::days(1)});
+
+ CHECK(*parseDateTimeStr<TimePointType>("1995-02-28T00:00:00Z") == TimePointType{793929600s});
+ CHECK(*parseDateTimeStr<TimePointType>("1995-03-01T00:00:00Z") == TimePointType{793929600s + std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("1996-02-28T00:00:00Z") == TimePointType{825465600s});
+ CHECK(*parseDateTimeStr<TimePointType>("1996-02-29T00:00:00Z") == TimePointType{825465600s + std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("2000-02-28T00:00:00Z") == TimePointType{951696000s});
+ CHECK(*parseDateTimeStr<TimePointType>("2000-02-29T00:00:00Z") == TimePointType{951696000s + std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("2100-02-28T00:00:00Z") == TimePointType{4107456000s});
+ CHECK(*parseDateTimeStr<TimePointType>("2100-03-01T00:00:00Z") == TimePointType{4107456000s + std::chrono::days(1)});
+
+ CHECK(*parseDateTimeStr<TimePointType>("2017-12-12T18:54:16Z") == TimePointType{1513104856s});
+ CHECK(*parseDateTimeStr<TimePointType>("2024-01-30T23:01:15Z") == TimePointType{1706655675s});
+ CHECK(*parseDateTimeStr<TimePointType>("2087-07-31T01:33:50Z") == TimePointType{3710453630s});
}
TEST_CASE("parseDateTimeStr() works correctly", "[parseDateTimeStr]") {
Review Comment:
I know it was missing before this PR, but it would be good to have some tests for `getDateTimeStr()`, too.
##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -63,6 +63,21 @@ constexpr char const* ListSFTP::TARGET_SYSTEM_TIMESTAMP_PRECISION_MINUTES;
constexpr char const* ListSFTP::ENTITY_TRACKING_INITIAL_LISTING_TARGET_TRACKING_TIME_WINDOW;
constexpr char const* ListSFTP::ENTITY_TRACKING_INITIAL_LISTING_TARGET_ALL_AVAILABLE;
+namespace {
+uint64_t createTimestamp(const std::optional<std::chrono::system_clock::time_point> time_point) {
+ if (!time_point)
+ return 0;
+ return std::chrono::duration_cast<std::chrono::milliseconds>(time_point->time_since_epoch()).count();
+}
+
+std::optional<std::chrono::system_clock::time_point> parseTimestamp(const uint64_t timestamp) {
+ if (timestamp == 0)
+ return std::nullopt;
+ return std::chrono::system_clock::time_point{std::chrono::milliseconds{timestamp}};
+}
Review Comment:
Minor quibble, but too many different things are called "timestamp" in this class. I would rename these helper functions to `toUnixTime` and `fromUnixTime`.
##########
libminifi/include/utils/TimeUtil.h:
##########
@@ -86,125 +86,49 @@ class SteadyClock : public Clock {
}
};
-/**
- * Returns a string based on TIME_FORMAT, converting
- * the parameter to a string
- * @param msec milliseconds since epoch
- * @returns string representing the time
- */
-inline std::string getTimeStr(uint64_t msec, bool enforce_locale = false) {
- char date[120];
- time_t second = (time_t) (msec / 1000);
- msec = msec % 1000;
- strftime(date, sizeof(date) / sizeof(*date), TIME_FORMAT, (enforce_locale == true ? gmtime(&second) : localtime(&second)));
-
- std::string ret = date;
- date[0] = '\0';
- sprintf(date, ".%03llu", (unsigned long long) msec); // NOLINT
-
- ret += date;
- return ret;
+inline std::string getTimeStr(std::chrono::system_clock::time_point tp) {
+ std::ostringstream stream;
+ date::to_stream(stream, TIME_FORMAT, std::chrono::floor<std::chrono::milliseconds>(tp));
+ return stream.str();
}
-inline time_t mkgmtime(struct tm* date_time) {
-#ifdef WIN32
- return _mkgmtime(date_time);
-#else
- static const int month_lengths[] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const int month_lengths_leap[] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const auto is_leap_year = [](int year) -> bool {
- return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
- };
- static const auto num_leap_days = [](int year) -> int {
- return (year - 1968) / 4 - (year - 1900) / 100 + (year - 1600) / 400;
- };
-
- int year = date_time->tm_year + 1900;
- time_t result = year - 1970;
- result *= 365;
- result += num_leap_days(year - 1);
-
- for (int i = 0; i < 12 && i < date_time->tm_mon; ++i) {
- result += is_leap_year(year) ? month_lengths_leap[i] : month_lengths[i];
- }
-
- result += date_time->tm_mday - 1;
- result *= 24;
-
- result += date_time->tm_hour;
- result *= 60;
-
- result += date_time->tm_min;
- result *= 60;
-
- result += date_time->tm_sec;
- return result;
-#endif
+template<class TimePoint>
+inline std::optional<TimePoint> parseDateTimeStr(const std::string& str) {
Review Comment:
I'm not sure if it makes sense for `parseDateTimeStr` and `getDateTimeStr` to support `local_seconds` as well as `sys_seconds`. We never use them with `local_seconds`; the format for `local_seconds` would not have the `Z` at the end; and it is easy to convert `local_seconds` to or from `sys_seconds` if we need to.
I would change them to take and return `sys_seconds`, so they can be plain functions instead of templates.
##########
libminifi/include/utils/TimeUtil.h:
##########
@@ -86,125 +86,49 @@ class SteadyClock : public Clock {
}
};
-/**
- * Returns a string based on TIME_FORMAT, converting
- * the parameter to a string
- * @param msec milliseconds since epoch
- * @returns string representing the time
- */
-inline std::string getTimeStr(uint64_t msec, bool enforce_locale = false) {
- char date[120];
- time_t second = (time_t) (msec / 1000);
- msec = msec % 1000;
- strftime(date, sizeof(date) / sizeof(*date), TIME_FORMAT, (enforce_locale == true ? gmtime(&second) : localtime(&second)));
-
- std::string ret = date;
- date[0] = '\0';
- sprintf(date, ".%03llu", (unsigned long long) msec); // NOLINT
-
- ret += date;
- return ret;
+inline std::string getTimeStr(std::chrono::system_clock::time_point tp) {
+ std::ostringstream stream;
+ date::to_stream(stream, TIME_FORMAT, std::chrono::floor<std::chrono::milliseconds>(tp));
+ return stream.str();
}
-inline time_t mkgmtime(struct tm* date_time) {
-#ifdef WIN32
- return _mkgmtime(date_time);
-#else
- static const int month_lengths[] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const int month_lengths_leap[] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const auto is_leap_year = [](int year) -> bool {
- return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
- };
- static const auto num_leap_days = [](int year) -> int {
- return (year - 1968) / 4 - (year - 1900) / 100 + (year - 1600) / 400;
- };
-
- int year = date_time->tm_year + 1900;
- time_t result = year - 1970;
- result *= 365;
- result += num_leap_days(year - 1);
-
- for (int i = 0; i < 12 && i < date_time->tm_mon; ++i) {
- result += is_leap_year(year) ? month_lengths_leap[i] : month_lengths[i];
- }
-
- result += date_time->tm_mday - 1;
- result *= 24;
-
- result += date_time->tm_hour;
- result *= 60;
-
- result += date_time->tm_min;
- result *= 60;
-
- result += date_time->tm_sec;
- return result;
-#endif
+template<class TimePoint>
+inline std::optional<TimePoint> parseDateTimeStr(const std::string& str) {
+ std::istringstream stream(str);
+ TimePoint tp;
+ date::from_stream(stream, "%Y-%m-%dT%H:%M:%SZ", tp);
+ if (stream.fail() || (stream.peek() && !stream.eof()))
+ return std::nullopt;
+ return tp;
}
-/**
- * Parse a datetime in yyyy-MM-dd'T'HH:mm:ssZ format
- * @param str the datetime string
- * @returns Unix timestamp
- */
-inline int64_t parseDateTimeStr(const std::string& str) {
- /**
- * There is no strptime on Windows. As long as we have to parse a single date format this is not so bad,
- * but if multiple formats will have to be supported in the future, it might be worth it to include
- * an strptime implementation from some BSD on Windows.
- */
- uint32_t year;
- uint8_t month;
- uint8_t day;
- uint8_t hours;
- uint8_t minutes;
- uint8_t seconds;
- int read = 0;
- if (sscanf(str.c_str(), "%4u-%2hhu-%2hhuT%2hhu:%2hhu:%2hhuZ%n", &year, &month, &day, &hours, &minutes, &seconds, &read) != 6) {
- return -1;
- }
- // while it is unlikely that read will be < 0, the conditional adds little cost for a little defensiveness.
- if (read < 0 || static_cast<size_t>(read) != str.size()) {
- return -1;
- }
-
- if (year < 1970U ||
- month > 12U ||
- day > 31U ||
- hours > 23U ||
- minutes > 59U ||
- seconds > 60U) {
- return -1;
- }
-
- struct tm timeinfo{};
- timeinfo.tm_year = year - 1900;
- timeinfo.tm_mon = month - 1;
- timeinfo.tm_mday = day;
- timeinfo.tm_hour = hours;
- timeinfo.tm_min = minutes;
- timeinfo.tm_sec = seconds;
- timeinfo.tm_isdst = 0;
-
- return static_cast<int64_t>(mkgmtime(&timeinfo));
+template<class TimePoint>
+inline std::optional<std::string> getDateTimeStr(TimePoint tp) {
+ std::ostringstream stream;
+ date::to_stream(stream, "%Y-%m-%dT%H:%M:%SZ", std::chrono::floor<std::chrono::milliseconds>(tp));
+ if (stream.fail())
+ return std::nullopt;
Review Comment:
Is it possible for this to fail? `getTimeStr()` doesn't check for failure, which seems correct to me. If it can't fail, then the return value could be `string` instead of `optional<string>`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org