refactor: Make adjusted time type safe#25786
Hidden character warning
Conversation
To be used in the next commit
maflcko
force-pushed
the
2208-time-type-
branch
from
fa8a30b to
eeee5ad
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
| */ | ||
| bool HaveTxsDownloaded() const { return nChainTx != 0; } | ||
|
|
||
| NodeSeconds Time() const |
There was a problem hiding this comment.
nit: This will end up renaming GetBlockTime(), right? (assuming that in the long run GetBlockTime() would be entirely replaced by Time())
we could avoid that with something like the following:
template <typename T = int64_t>
auto GetBlockTime() const -> T
{
static_assert(std::is_same<T, int64_t>::value || std::is_same<T, NodeSeconds>::value);
if constexpr (std::is_same<T, int64_t>::value) {
return (T)nTime;
} else {
return NodeSeconds{std::chrono::seconds{nTime}};
}
}There was a problem hiding this comment.
I picked a different name because it felt weird to repeat the class type in the method again. That is, calling block.GetBlockTime() seems better replaced by block.GetTime() or block.Time().
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK eeee5ad. Confirmed type changes and equivalent code changes only.
It's pretty hard to keep tract of all the different time functions. I think it would be really nice if all of the deprecated time functions could just be eliminated. Also if there are time functions that are never used (like ::Now()?) or only infrequently used I think it would be nice to drop them from util/time.h and just declare them locally close to where they are used to avoid having so many confusing similarly named util functions.
I am doing this in other pull requests. For example, |
5 participants
This makes follow-ups easier to review. Also, it makes sense by itself.