[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1225: MINIFICPP-1688: When storing time durations we should use std::chrono…

[GitBox <gi...@apache.org>]

fgerlits commented on a change in pull request #1225:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1225#discussion_r784141713



##########
File path: libminifi/include/sitetosite/Peer.h
##########
@@ -155,12 +158,11 @@ class SiteToSitePeer : public org::apache::nifi::minifi::io::BaseStream {
       : stream_(nullptr),
         host_(host),
         port_(port),
-        timeout_(30000),
-        yield_expiration_(0),
+        timeout_(std::chrono::seconds(30)),
         logger_(core::logging::LoggerFactory<SiteToSitePeer>::getLogger()) {
     url_ = "nifi://" + host_ + ":" + std::to_string(port_);
-    yield_expiration_ = 0;
-    timeout_ = 30000;  // 30 seconds
+    yield_expiration_ = std::chrono::system_clock::time_point();
+    timeout_ = std::chrono::seconds(30);  // 30 seconds

Review comment:
       this comment is no longer needed

##########
File path: libminifi/include/utils/TimeUtil.h
##########
@@ -197,6 +193,106 @@ inline bool getDateTimeStr(int64_t unix_timestamp, std::string& date_time_str) {
   return true;
 }
 
+namespace details {
+
+template<class Duration>
+bool unit_matches(const std::string&) {
+  return false;
+}
+
+template<>
+inline bool unit_matches<std::chrono::nanoseconds>(const std::string& unit) {
+  return unit == "ns" || unit == "nano" || unit == "nanos" || unit == "nanoseconds";

Review comment:
       Can you add "nanosecond", please?  I know it's old code, and we are unlikely to set a property to "1 nanosecond", but all the other units have the singular version, too.

##########
File path: libminifi/include/core/ProcessorConfig.h
##########
@@ -32,8 +32,8 @@ namespace core {
 
 #define DEFAULT_SCHEDULING_STRATEGY "TIMER_DRIVEN"
 #define DEFAULT_SCHEDULING_PERIOD_STR "1 sec"
-#define DEFAULT_SCHEDULING_PERIOD_MILLIS 1000
-#define DEFAULT_RUN_DURATION 0
+constexpr std::chrono::milliseconds DEFAULT_SCHEDULING_PERIOD_MILLIS{1000};
+constexpr std::chrono::nanoseconds DEFAULT_RUN_DURATION{0};
 #define DEFAULT_MAX_CONCURRENT_TASKS 1
 #define DEFAULT_YIELD_PERIOD_SECONDS 1

Review comment:
       `DEFAULT_YIELD_PERIOD_SECONDS` could also be a `constexpr std::chrono::seconds` instead of a macro

##########
File path: extensions/windows-event-log/CollectorInitiatedSubscription.cpp
##########
@@ -39,6 +39,8 @@
 #pragma comment(lib, "wevtapi.lib")
 #pragma comment(lib, "Wecapi.lib")
 
+using namespace std::chrono_literals;  // NOLINT(build/namespaces)

Review comment:
       this could be `std::literals::chrono_literals`, too (and there is another one in `ConsumeWindowsEventLog.cpp`)




-- 
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