[Spice-devel] [PATCH 09/11] utils: add red_get_monotonic_time()

[Frediano Ziglio]

> 
> On Thu, 29 Oct 2015, Frediano Ziglio wrote:
> [...]
> > +/* FIXME: consider g_get_monotonic_time (), but in microseconds */
> > +static inline red_time_t red_get_monotonic_time(void)
> > +{
> > +    struct timespec time;
> > +
> > +    clock_gettime(CLOCK_MONOTONIC, &time);
> > +    return (red_time_t) time.tv_sec * (1000 * 1000 * 1000) + time.tv_nsec;
> > +}
> 
> I think the name should be more generic as I see no reason why this
> function could not also replace get_time_stamp() (the signed/unsigned
> difference does not seem insurmountable), and various clock_gettime()
> calls in mjpeg_encoder.c and in red_channel.c.
>

I agree, this code (clock_gettime(MONOTONIC) + computation) is duplicated
quite a lot. Yes, get_time_stamp is exactly the same. int64_t/uint64_t in
this case is not that different but for a generic function I would prefer
the signed version as unsigned could cause some overflows (well, is much
likely to cause them). The range is enough to cover any possible value from
red_get_monotonic_time/get_time_stamp.
 
> It would also be nice to have a NANO_SECOND constant instead of '1000 *
> 1000 * 1000' in one place, '1000000000ULL' in another (notice the LL
> btw), etc.
> 
> Also '30 * NANO_SECOND' would be clearer than '30000000000ULL'.
>

I don't think is much more readable. Personally NANO_SECOND seems a time
measure instead of a time conversion constant I would prefer something like
NANOSECONDS_PER_SECOND but well, it's obviously 1000 * 1000 * 1000 !

Note that in the patch the LL is useless as the conversion is explicit
on the first factor.
 
> I'm open to naming variations but it should be something that can
> readily be adapted to get the duration of one second in milliseconds,
> MILLI_SECOND, in microseconds, MICRO_SECOND, or the duration of a
> millisecond in nanoseconds (for conversions), NANO_MS or
> NANO_MILLISECOND.
>

The patch was pushed but I don't see any drawback in improving it.

Frediano