Merged
Conversation
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
maflcko
force-pushed
the
1905-typeTime
branch
5 times, most recently
from
2d19686 to
fac4143
Compare
Contributor
|
Concept ACK. |
Contributor
|
Concept ACK |
promag
reviewed
May 20, 2019
src/test/util_tests.cpp
Outdated
Contributor
There was a problem hiding this comment.
Why the loop? Isn't enough one loop after MilliSleep(1)?
Member
Author
There was a problem hiding this comment.
Yeah, those are unit tests. Probably enough to not have them at all 🤷♀️
src/util/time.h
Outdated
Member
Author
There was a problem hiding this comment.
This is not possible and would lead to compile errors:
util/time.cpp: In function ‘std::__cxx11::string FormatISO8601DateTime(int64_t)’:
util/time.cpp:84:9: error: ‘gmtime_r’ was not declared in this scope
if (gmtime_r(&time_val, &ts) == nullptr) {
^~~~~~~~
util/time.cpp:84:9: note: suggested alternative: ‘gmtime_s’
if (gmtime_r(&time_val, &ts) == nullptr) {
^~~~~~~~
gmtime_s
util/time.cpp: In function ‘std::__cxx11::string FormatISO8601Date(int64_t)’:
util/time.cpp:97:9: error: ‘gmtime_r’ was not declared in this scope
if (gmtime_r(&time_val, &ts) == nullptr) {
^~~~~~~~
util/time.cpp:97:9: note: suggested alternative: ‘gmtime_s’
if (gmtime_r(&time_val, &ts) == nullptr) {
^~~~~~~~
gmtime_s
Makefile:11303: recipe for target 'util/libbitcoin_util_a-time.o' failed
laanwj
reviewed
May 20, 2019
src/util/time.h
Outdated
Member
There was a problem hiding this comment.
Need to keep the information here that GetTimeMicros() and GetTimeMillis() aren't mocked
Member
Author
There was a problem hiding this comment.
Their docstring already says Returns the system time. Anything I should add?
Member
There was a problem hiding this comment.
I think it's better to explicitly mention that they ignore the mocked time. My implicit assumption while reading the documentation would be that mocktime would affect everyting.
Member
Author
There was a problem hiding this comment.
Yeah, makes sense. Fixed up documentation a bit
Contributor
|
utACK fa01366. |
Member
|
utACK fa01366 |
laanwj
added a commit
that referenced
this pull request
May 29, 2019
fa01366 util: Add type safe GetTime (MarcoFalke) Pull request description: There are basically two ways to get the time in Bitcoin Core: * get the system time (via `GetSystemTimeInSeconds` or `GetTime{Millis,Micros}`) * get the mockable time (via `GetTime`) Both return the same type (a plain int). This can lead to (test-only) bugs such as 99464bc. Fix that by deprecating `GetTime` and adding a `GetTime<>` that returns the mockable time in a non-int type. The new util function is currently unused, but new code should it where possible. ACKs for commit fa0136: promag: utACK fa01366. Tree-SHA512: efab9c463f079fd8fd3030c479637c7b1e8be567a881234bd0f555c8f87e518e3b43ef2466128103db8fc40295aaf24e87ad76d91f338c631246fc703477e95c
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Mar 8, 2020
Summary: This is a backport of [[bitcoin/bitcoin#16046 | PR16046]] Test Plan: ninja check ninja bench-bitcoin Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D5445
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Apr 8, 2020
fa01366 util: Add type safe GetTime (MarcoFalke) Pull request description: There are basically two ways to get the time in Bitcoin Core: * get the system time (via `GetSystemTimeInSeconds` or `GetTime{Millis,Micros}`) * get the mockable time (via `GetTime`) Both return the same type (a plain int). This can lead to (test-only) bugs such as 99464bc. Fix that by deprecating `GetTime` and adding a `GetTime<>` that returns the mockable time in a non-int type. The new util function is currently unused, but new code should it where possible. ACKs for commit fa0136: promag: utACK fa01366. Tree-SHA512: efab9c463f079fd8fd3030c479637c7b1e8be567a881234bd0f555c8f87e518e3b43ef2466128103db8fc40295aaf24e87ad76d91f338c631246fc703477e95c
ftrader
pushed a commit
to bitcoin-cash-node/bitcoin-cash-node
that referenced
this pull request
May 19, 2020
Summary: This is a backport of [[bitcoin/bitcoin#16046 | PR16046]] Test Plan: ninja check ninja bench-bitcoin Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D5445
random-zebra
added a commit
to PIVX-Project/PIVX
that referenced
this pull request
May 5, 2021
f3f3d6c util: fix compilation with mingw-w64 7.0.0 (Fuzzbawls) f24da7d util: Avoid potential uninitialized read in FormatISO8601DateTime (Fuzzbawls) 120a12a util: Add type safe GetTime (Fuzzbawls) be48766 Fix for utiltime to compile with msvc. (Aaron Clauson) 2833406 Avoid std::locale/imbue in DateTimeStrFormat (Pieter Wuille) 3331c25 Format automatic wallet backup file names in ISO 8601 basic format (Fuzzbawls) c32a208 Format timestamps using ISO 8601 formatting (e.g. "2018-02-28T12:34:56Z") (practicalswift) f8bd173 [logging] log system time and mock time (John Newbery) Pull request description: This is a backport of a collection of upstream PRs to bring our time utility more up to date. The following upstream PRs are included: - bitcoin#10383 - bitcoin#12567 - bitcoin#12973 - bitcoin#13031 - bitcoin#16046 - bitcoin#18162 - bitcoin#18358 The two user-facing notable changes here have been documented in the release notes template. I've also connected the functionality that `-logtimemicros` was supposed to provide. ACKs for top commit: furszy: code review ACK f3f3d6c random-zebra: utACK f3f3d6c after rebase, and merging... Tree-SHA512: 64f9cc1f7fc65c192f3b3ab869c057a81e007cf0fae82ecd23993c6b51830e4bc80656c699ba40bb8e019115a24d1ea88a618be0c0d7112749d9ce125d62ce44
gades
pushed a commit
to cosanta/cosanta-core
that referenced
this pull request
Jun 30, 2021
fa01366 util: Add type safe GetTime (MarcoFalke) Pull request description: There are basically two ways to get the time in Bitcoin Core: * get the system time (via `GetSystemTimeInSeconds` or `GetTime{Millis,Micros}`) * get the mockable time (via `GetTime`) Both return the same type (a plain int). This can lead to (test-only) bugs such as 99464bc. Fix that by deprecating `GetTime` and adding a `GetTime<>` that returns the mockable time in a non-int type. The new util function is currently unused, but new code should it where possible. ACKs for commit fa0136: promag: utACK fa01366. Tree-SHA512: efab9c463f079fd8fd3030c479637c7b1e8be567a881234bd0f555c8f87e518e3b43ef2466128103db8fc40295aaf24e87ad76d91f338c631246fc703477e95c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are basically two ways to get the time in Bitcoin Core:
GetSystemTimeInSecondsorGetTime{Millis,Micros})GetTime)Both return the same type (a plain int). This can lead to (test-only) bugs such as 99464bc.
Fix that by deprecating
GetTimeand adding aGetTime<>that returns the mockable time in a non-int type. The new util function is currently unused, but new code should it where possible.