Skip to content

Commit e0ff55a

Browse files
committed
Merge bitcoin/bitcoin#24871: refactor: Simplify GetTime
0000a63 Simplify GetTime (MarcoFalke) Pull request description: The implementation of `GetTime` is confusing: * The value returned by `GetTime` is assumed to be equal to `GetTime<std::chrono::seconds>()`. Both are mockable and the only difference is return type, the value itself is equal. However, the implementation does not support this assumption. * On some systems, `time_t` might be a signed 32-bit integer (https://en.cppreference.com/w/c/chrono/time), thus breaking in the year 2038, whereas `GetTime<std::chrono::seconds>` does not. Also, `time_t` might be `-1` "on error", where "error" is unspecified. * `GetTime<std::chrono::seconds>` calls `GetTimeMicros`, which calls `GetSystemTime`, which calls `std::chrono::system_clock::now`, which doesn't have the above issues. See https://en.cppreference.com/w/cpp/chrono/system_clock/now * `GetTimeMicros` and the internal-only `GetSystemTime` will likely be renamed (to clarify they are the non-mockable non-monotonic system time) or removed in the future to be replaced by appropriate `std::chrono::time_point<Clock>` getters. Fix all issues by: * making `GetTime()` an alias for `GetTime<std::chrono::seconds>().count()`. * inlining the needed parts of `GetSystemTime` directly instead of needlessly increasing the function call stack with functions that are likely to be removed in the future. ACKs for top commit: martinus: Code review, untested ACK 0000a63. By the way strictly speaking `std::chrono::system_clock` is only guaranteed to be based on the unix epoch starting with C++20: https://en.cppreference.com/w/cpp/chrono/system_clock theStack: Code-review ACK 0000a63 Tree-SHA512: f751ba740e0da65537be800e9414dd02282d9f04c0b0fb986a36546f257d0b888d8688653cdda5d355ec832c0e09d866922d9161b1ccd33485c1c92c5d1e802f
2 parents 8d3743a + 0000a63 commit e0ff55a

File tree

1 file changed

+6
-13
lines changed

1 file changed

+6
-13
lines changed

src/util/time.cpp

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,6 @@ void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread
2323

2424
static std::atomic<int64_t> nMockTime(0); //!< For testing
2525

26-
int64_t GetTime()
27-
{
28-
int64_t mocktime = nMockTime.load(std::memory_order_relaxed);
29-
if (mocktime) return mocktime;
30-
31-
time_t now = time(nullptr);
32-
assert(now > 0);
33-
return now;
34-
}
35-
3626
bool ChronoSanityCheck()
3727
{
3828
// std::chrono::system_clock.time_since_epoch and time_t(0) are not guaranteed
@@ -80,11 +70,12 @@ template <typename T>
8070
T GetTime()
8171
{
8272
const std::chrono::seconds mocktime{nMockTime.load(std::memory_order_relaxed)};
83-
84-
return std::chrono::duration_cast<T>(
73+
const auto ret{
8574
mocktime.count() ?
8675
mocktime :
87-
std::chrono::microseconds{GetTimeMicros()});
76+
std::chrono::duration_cast<T>(std::chrono::system_clock::now().time_since_epoch())};
77+
assert(ret > 0s);
78+
return ret;
8879
}
8980
template std::chrono::seconds GetTime();
9081
template std::chrono::milliseconds GetTime();
@@ -129,6 +120,8 @@ int64_t GetTimeSeconds()
129120
return int64_t{GetSystemTime<std::chrono::seconds>().count()};
130121
}
131122

123+
int64_t GetTime() { return GetTime<std::chrono::seconds>().count(); }
124+
132125
std::string FormatISO8601DateTime(int64_t nTime) {
133126
struct tm ts;
134127
time_t time_val = nTime;

0 commit comments

Comments
 (0)