refactor: Make adjusted time type safe #25786

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2208-time-type-💷 changing 11 files +36 −16

MarcoFalke commented at 12:48 PM on August 5, 2022: member

This makes follow-ups easier to review. Also, it makes sense by itself.
Add time helpers
To be used in the next commit
fa3be799fe
Make adjusted time type safe eeee5ada23
MarcoFalke force-pushed on Aug 5, 2022
fanquake requested review from dergoegge on Aug 5, 2022
DrahtBot added the label Refactoring on Aug 5, 2022
DrahtBot commented at 4:51 PM on August 5, 2022: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #25862 (refactor, kernel: Remove gArgs accesses from dbwrapper and txdb by ryanofsky)
- #25781 (Remove almost all blockstorage globals by MarcoFalke)
- #25704 (refactor: Remove almost all validation option globals by MarcoFalke)
- #25623 ([kernel 3e/n] Decouple CDBWrapper and its kernel users from ArgsManager by dongcarl)
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.
DrahtBot cross-referenced this on Aug 5, 2022 from issue Remove almost all blockstorage globals by MarcoFalke
DrahtBot cross-referenced this on Aug 5, 2022 from issue refactor: Remove almost all validation option globals by MarcoFalke

in src/chain.h:279 in eeee5ada23

 275 | @@ -275,6 +276,11 @@ class CBlockIndex
 276 |       */
 277 |      bool HaveTxsDownloaded() const { return nChainTx != 0; }
 278 |  
 279 | +    NodeSeconds Time() const

dergoegge commented at 6:06 PM on August 5, 2022:

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}};
        }
    }

MarcoFalke commented at 6:55 PM on August 18, 2022:

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().

dergoegge commented at 6:17 PM on August 5, 2022: member

Concept ACK
DrahtBot cross-referenced this on Aug 5, 2022 from issue [kernel 3e/n] Decouple `CDBWrapper` and its kernel users from `ArgsManager` by dongcarl
DrahtBot cross-referenced this on Aug 17, 2022 from issue refactor, kernel: Remove gArgs accesses from dbwrapper and txdb by ryanofsky
ryanofsky approved
ryanofsky commented at 6:29 PM on August 18, 2022: contributor

Code review ACK eeee5ada23f2a71d245671556b6ecfdaabfeddf4. 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.
MarcoFalke commented at 6:58 PM on August 18, 2022: member

if there are time functions that are never used (like ::Now()?) ...

::Now is used, but can only be used by passing the template argument, so you'd have to grep for: git grep '\<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

I am doing this in other pull requests. For example, GetTimeMillis in #25499.
fanquake merged this on Aug 22, 2022
fanquake closed this on Aug 22, 2022
MarcoFalke deleted the branch on Aug 22, 2022
sidhujag referenced this in commit fdff3afc15 on Aug 22, 2022
bitcoin locked this on Aug 22, 2023

Contributors

Labels

Linked (view graph)

#25499 Use steady clock for all millis bench logging

refactor: Make adjusted time type safe #25786

Conflicts