refactor: Make adjusted time type safe #25786

1. 
   MarcoFalke commented at 12:48 pm on August 5, 2022: member

   This makes follow-ups easier to review. Also, it makes sense by itself.
2. Add time helpers

   To be used in the next commit

   fa3be799fe
3. Make adjusted time type safe eeee5ada23
4. MarcoFalke force-pushed on Aug 5, 2022
5. fanquake requested review from dergoegge on Aug 5, 2022
6. DrahtBot added the label Refactoring on Aug 5, 2022
7. 
   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.

8. 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:

    0    template <typename T = int64_t>
    1    auto GetBlockTime() const -> T
    2    {
    3        static_assert(std::is_same<T, int64_t>::value || std::is_same<T, NodeSeconds>::value);
    4
    5        if constexpr (std::is_same<T, int64_t>::value) {
    6            return (T)nTime;
    7        } else {
    8            return NodeSeconds{std::chrono::seconds{nTime}};
    9        }
   10    }
   

   ---

   

   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().
9. 
   dergoegge commented at 6:17 pm on August 5, 2022: member

   Concept ACK
10. ryanofsky approved
11. 
    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.

12. 
    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.

13. fanquake merged this on Aug 22, 2022
14. fanquake closed this on Aug 22, 2022

    ---

15. MarcoFalke deleted the branch on Aug 22, 2022
16. sidhujag referenced this in commit fdff3afc15 on Aug 22, 2022
17. bitcoin locked this on Aug 22, 2023

Contributors
MarcoFalke DrahtBot dergoegge ryanofsky

Labels
Refactoring