Re: [Qemu-block] [PATCH] Replacing (and removing) get_ticks_per_sec() function with NANOSECONDS_PER_SECOND Signed-off-by: Rutuja Shah <address@hidden>

[Christian Borntraeger]

On 03/10/2016 08:30 PM, address@hidden wrote:
> From: Rutuja Shah <address@hidden>
[...]
> -    s->tick_offset_vmstate = s->tick_offset + delta / get_ticks_per_sec();
> +    s->tick_offset_vmstate = s->tick_offset + delta / NANOSECONDS_PER_SECOND;
[...]

While technically correct, I do not like these changes. The interfaces expect 
"ticks",
and the fact that this happens to be a nanosecond does not help regarding 
readability.

Lets look at the snippet from above. The variable is called TICK_offset_vmstate.
so, the first line (-) makes sense when reading, the  2nd line (+) does not.
A reader that does not know the timer code will ask itself: "Why do we use 
NANOSECONDS_PER_SECOND to calculate ticks?" The reader has to know that a tick
is a nanosecond to understand that line. So the cleanup makes the code harder to
read in my opinion. 

If - for some reason - we want to replace a function with a MACRO, then
please introduce TICKS_PER_SECOND which just feels better when reading the code.
It would also fix the >80 chars per line problem.
On the other hand get_ticks_per_sec is a static inline function:
- it will get inlined, no overhead over a macro
- it will add type safety if we ever change things (e.g. somebody might 
introduce
a tick_t)

So I would prefer to not change things.

Christian