refactor: Replace GetTimeMicros by SystemClock #27233
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Concept ACK |
|
Concept ACK |
willcl-ark
left a comment
willcl-ark
left a comment
There was a problem hiding this comment.
tACK faf3f12
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.
| using SteadyMilliseconds = std::chrono::time_point<std::chrono::steady_clock, std::chrono::milliseconds>; | ||
| using SteadyMicroseconds = std::chrono::time_point<std::chrono::steady_clock, std::chrono::microseconds>; | ||
|
|
||
| using SystemClock = std::chrono::system_clock; |
There was a problem hiding this comment.
nit: as we only use this in one single place (logging.cpp) it almost seems to make more sense just to drop this and directly use std::chrono::system_clock there? Although having SteadyClock and SystemClock as the two options perhaps prefereable.
There was a problem hiding this comment.
If this is used in more places in the future, it seems convenient to have NodeClock, SteadyClock, and SystemClock in the same header?
john-moffett
left a comment
john-moffett
left a comment
There was a problem hiding this comment.
ACK faf3f12 changes, but left a comment for the existing code.
| const auto now_seconds{std::chrono::time_point_cast<std::chrono::seconds>(now)}; | ||
| strStamped = FormatISO8601DateTime(TicksSinceEpoch<std::chrono::seconds>(now_seconds)); | ||
| if (m_log_time_micros) { | ||
| strStamped.pop_back(); |
There was a problem hiding this comment.
Very unlikely, but could this result in undefined behavior? 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?
There was a problem hiding this comment.
Yes, I am happy to review a pull doing this.
There was a problem hiding this comment.
gmtime can also be removed once we have C++20, but not sure if and when that happens
faf3f12 refactor: Replace GetTimeMicros by SystemClock (MarcoFalke) Pull request description: 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. ACKs for top commit: willcl-ark: tACK faf3f12 john-moffett: ACK faf3f12 changes, but left a comment for the existing code. Tree-SHA512: 069e6ef26467a469f128b98a4aeb334f141742befd7880cb3a7d280480e9f0684dc0686fa6a828cdcb3d11943ae5c7f8ad5d9d9dab4c668be85e5d28c78cd489
…non-empty to avoid undefined behavior 73f4eb5 Check that the Timestamp String is valid (John Moffett) Pull request description: Follow-up to bitcoin/bitcoin#27233 The current `FormatISO8601DateTime` function will return an empty string if it encounters an error when converting the `int64_t` seconds-since-epoch to a formatted date time. In the unlikely case that happens, here `strStamped.pop_back()` would be undefined behavior. ACKs for top commit: MarcoFalke: lgtm ACK 73f4eb5 stickies-v: ACK 73f4eb5 Tree-SHA512: 089ed639c193deb98870a8385039b31b4baed821ea907937bfc6f65a5b0981bbf8284b2afec81b2d0a922e2340716b48cf55349640eb6b8c311ef7af25abc361
…y to avoid undefined behavior 73f4eb5 Check that the Timestamp String is valid (John Moffett) Pull request description: Follow-up to bitcoin#27233 The current `FormatISO8601DateTime` function will return an empty string if it encounters an error when converting the `int64_t` seconds-since-epoch to a formatted date time. In the unlikely case that happens, here `strStamped.pop_back()` would be undefined behavior. ACKs for top commit: MarcoFalke: lgtm ACK 73f4eb5 stickies-v: ACK 73f4eb5 Tree-SHA512: 089ed639c193deb98870a8385039b31b4baed821ea907937bfc6f65a5b0981bbf8284b2afec81b2d0a922e2340716b48cf55349640eb6b8c311ef7af25abc361
6 participants
It is unclear from the name that
GetTimeMicrosreturns the system time. Also, it is not using the type-safestd::chronotypes.Fix both issues by replacing it with
SystemClockin the only place it is used.This refactor should not change behavior.