Page MenuHomePhabricator

Simplify GetTime
ClosedPublic

Authored by PiRK on Jul 22 2022, 08:51.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC7177049f17c6: Simplify GetTime
Summary

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.

This is a backport of core#24871

Depends on D11790

Test Plan

ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Jul 22 2022, 08:51
This revision is now accepted and ready to land.Jul 22 2022, 09:20
This revision was automatically updated to reflect the committed changes.