It is unclear from the name that GetTimeMicros returns the system time. Also, it is not using the type-safe std::chrono types.
Fix both issues by replacing it with SystemClock in the only place it is used.
This refactor should not change behavior.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | willcl-ark, john-moffett |
| Concept ACK | dergoegge, fanquake |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
28@@ -29,6 +29,8 @@ using SteadySeconds = std::chrono::time_point<std::chrono::steady_clock, std::ch
29 using SteadyMilliseconds = std::chrono::time_point<std::chrono::steady_clock, std::chrono::milliseconds>;
30 using SteadyMicroseconds = std::chrono::time_point<std::chrono::steady_clock, std::chrono::microseconds>;
31
32+using SystemClock = std::chrono::system_clock;
std::chrono::system_clock there? Although having SteadyClock and SystemClock as the two options perhaps prefereable.
NodeClock, SteadyClock, and SystemClock in the same header?
tACK faf3f1242
As I was unfamiliar with this code it was a little counter-intuitive to me at first that we truncate the time to seconds, before re-adding the microseconds if m_log_time_micros was set, but sure enough it mirrors the old behaviour.
I also left one tiny nit/comment which doesn’t need to be addressed.
353- strStamped = FormatISO8601DateTime(nTimeMicros/1000000);
354+ const auto now{SystemClock::now()};
355+ const auto now_seconds{std::chrono::time_point_cast<std::chrono::seconds>(now)};
356+ strStamped = FormatISO8601DateTime(TicksSinceEpoch<std::chrono::seconds>(now_seconds));
357 if (m_log_time_micros) {
358 strStamped.pop_back();
FormatISO8601DateTime can theoretically return an empty string if it’s fed, eg, an extremely large value and gmtime fails. While I can’t see any reason for that happening (the libstdc++ ::max() values I checked are reasonable) maybe best to avoid it just in case?