thank you @curious-george-rk!
I think this is fine to go that path, just wanted to raise the alternative (which I thought of going through but had second thoughts if it worth the trouble):
After a full refactor, will solve the issue, but will be harder to maintain and prevent future return to time being switched context (like you start with mstime_t, convert it to long long for some reason, and than to ustime_t).
I think although there is less chance of it after the refactor, we could also enforce these types eg:

typedef struct {long long ms;} mstime_t; /* millisecond time type. */
typedef struct {long long us;} ustime_t; /* microsecond time type. */

It would mean changing more locations, and maybe some of the code will start look more like:

ttl.ms += commandTimeSnapshot().ms;

but it would ensure future "saftiness".

I think other than the verbosity, it is probably a good idea to go to it long-term for the type enforcement. However, I don't really see a good way to do that incrementally since all the references would need to be updated at once, which would probably be a lot of changes in the short-term.

I think other than the verbosity, it is probably a good idea to go to it long-term for the type enforcement. However, I don't really see a good way to do that incrementally since all the references would need to be updated at once, which would probably be a lot of changes in the short-term.

Yes. As I wrote "It would mean changing more locations" I probably should have been clear stating "ALL locations"

but @curious-george-rk the work is very technical and not complex. I just think that we need to get some view of how the code would look like and understand what "sacrifices" we will have taking the enforced approach

The more I think about it, the less I like the wrapped struct method. Particularly, I;m not a fan of the way the variable initialization looks
`
To me,

is more readable than

Ultimately, I think these changes are meant to enhance the code readability, and the typed version sacrifices some of that.

Maybe some others can also weigh in on which approach they like more?

The more I think about it, the less I like the wrapped struct method. Particularly, I;m not a fan of the way the variable initialization looks ` To me, is more readable than

Ultimately, I think these changes are meant to enhance the code readability, and the typed version sacrifices some of that.

Maybe some others can also weigh in on which approach they like more?

@curious-george-rk agree that this will make the code less readable and uglier, which was the reason I "had second thoughts"... (these are the cases I miss C++ :) )
However I do not agree this change was aiming just readability. the change was meant to make sure we avoid mistakes of accidentally converting use of mstine_t to ustime_t (eg #3255)

I do agree that going with the wrapped struct method is probably something we would like to do a major decision for.
Personally I am O.K with making this just a cosmetic change, but I would love to hear other thoughts as well.
@valkey-io/core-team WDYT?

typedef struct {long long ms;} mstime_t; /* millisecond time type. */
typedef struct {long long us;} ustime_t; /* microsecond time type. */

It would mean changing more locations, and maybe some of the code will start look more like:

ttl.ms += commandTimeSnapshot().ms;

This is called nominal types. NASA's Mars Climate Orbiter crashed due to a bug like this: there was a calculation in pound-force seconds for something expected to be in newton-seconds (metric vs non-metric units). Maybe a nominal type system could have prevented it. :)

Nominal types can be pushed very far to encode all kinds of semantic differences to make types incompatible with each other.

For example, we could encode the difference between an absolute timestamp and delta (duration). Even if both are in microseconds, they can't be used interchangeably. The rules would be something like:

    delta      = abs_time_2 - abs_time_1  // a duration
    abs_time_2 = abs_time_1 + delta       // e.g. an expiration time in the future
    x          = abs_time_1 + abs_time_2  // doesn't make any sense; forbid it

We have yet another difference: unix time vs monotonic time. Even if both are in the same unit, their zero don't represent the same point in time and they may drift relative each other, so we can't mix them arbitrarily, for example taking the diff between a ustime (from gettimeofday) and a monotime doesn't make sense. We can add a monotonic delta to a unix timestamp though. In this recent optimization, do just this to avoid calling gettimeofday() too often by instead adding a monotonic delta:

• Optimize ustime() with monotonic delta #3193

Here, monotime and ustime_t are used together, but they're both 64-bit integers anyway. A stricter type system could be used, but it wouldn't help us reason about the more important problem here: how much the precision is affected by drift of the monotonic clock, which is based on calibration (measuring of hardware-clock ticks per microsecond that we do at startup).

Nominal types can probably be pushed very far, but it would also make it cumbersome to read and write code if we go too far in that direction. I don't think we shall introduce the struct variant. I think we should stay fairly close to common C code (e.g. the style used in standard library types and functions). Typedefs for ustime_t and similar units is fine for readability although it doesn't really improve the strictness in compile-time type checking, but it follows the style of the standard time_t.

typedef struct {long long ms;} mstime_t; /* millisecond time type. */
typedef struct {long long us;} ustime_t; /* microsecond time type. */

It would mean changing more locations, and maybe some of the code will start look more like:

ttl.ms += commandTimeSnapshot().ms;

This is called nominal types. NASA's Mars Climate Orbiter crashed due to a bug like this: there was a calculation in pound-force seconds for something expected to be in newton-seconds (metric vs non-metric units). Maybe a nominal type system could have prevented it. :)

Nominal types can be pushed very far to encode all kinds of semantic differences to make types incompatible with each other.

For example, we could encode the difference between an absolute timestamp and delta (duration). Even if both are in microseconds, they can't be used interchangeably. The rules would be something like:

    delta      = abs_time_2 - abs_time_1  // a duration
    abs_time_2 = abs_time_1 + delta       // e.g. an expiration time in the future
    x          = abs_time_1 + abs_time_2  // doesn't make any sense; forbid it

We have yet another difference: unix time vs monotonic time. Even if both are in the same unit, their zero don't represent the same point in time and they may drift relative each other, so we can't mix them arbitrarily, for example taking the diff between a ustime (from gettimeofday) and a monotime doesn't make sense. We can add a monotonic delta to a unix timestamp though. In this recent optimization, do just this to avoid calling gettimeofday() too often by instead adding a monotonic delta:

• Optimize ustime() with monotonic delta #3193

Here, monotime and ustime_t are used together, but they're both 64-bit integers anyway. A stricter type system could be used, but it wouldn't help us reason about the more important problem here: how much the precision is affected by drift of the monotonic clock, which is based on calibration (measuring of hardware-clock ticks per microsecond that we do at startup).

Nominal types can probably be pushed very far, but it would also make it cumbersome to read and write code if we go too far in that direction. I don't think we shall introduce the struct variant. I think we should stay fairly close to common C code (e.g. the style used in standard library types and functions). Typedefs for ustime_t and similar units is fine for readability although it doesn't really improve the strictness in compile-time type checking, but it follows the style of the standard time_t.

@zuiderkwast I read you paper :) but I did not follow the bottom line? Do you think that my suggestion placing a strict type enforcement on time types IS a good thing to have (even if it complicates the code and readability)?

I don't think we shall introduce the struct variant. I think we should stay fairly close to common C code (e.g. the style used in standard library types and functions). Typedefs for ustime_t and similar units is fine for readability although it doesn't really improve the strictness in compile-time type checking, but it follows the style of the standard time_t.

@zuiderkwast I read you paper :) but I did not follow the bottom line? Do you think that my suggestion placing a strict type enforcement on time types IS a good thing to have (even if it complicates the code and readability)?

Paper, hehe.

Bottom line: I don't think we shall introduce the struct variant.

Codecov Report

❌ Patch coverage is 95.83333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.94%. Comparing base (fc217fc) to head (8136c62).
⚠️ Report is 3 commits behind head on unstable.

@@             Coverage Diff              @@
##           unstable    #3252      +/-   ##
============================================
- Coverage     75.05%   74.94%   -0.12%     
============================================
  Files           129      129              
  Lines         71553    71553              
============================================
- Hits          53705    53624      -81     
- Misses        17848    17929      +81