On 03/10/2016 08:30 PM, rutu.shah...@gmail.com wrote: > From: Rutuja Shah <rutu.shah...@gmail.com> [...] > - 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
