[Spice-devel] [spice v3] utils: Remove the LL suffix from NSEC_PER_SEC

[Francois Gouget]

Uri Lublin wrote:
> When the variable is 64 bit, you can use a 64bit macro for printing,
> like PRId64.

Wrong. Spice will fail to produce a 64 bit library if you add this 
anywhere:

    uint64_t foo = 1234;
    spice_debug("foo=%" PRId64, foo / NSEC_PER_SEC);

Knowing that the variable is 64 bit is not sufficient to know what 
format to use. And once a developer has figured out that the time 
constants are long longs he'll get an error with this:

    uint64_t bar = 1234;
    spice_debug("bar=%lld", bar / NSEC_PER_MICROSEC);

These are traps. It's on the same level as:

#define ONE 2

Useless inconsistencies and misdirections.


The principle of least surprise would dictate that for maintainable 
code:

  1. A set of related constants should all have the same type.

  2. If at all possible constants should not force their type on that 
     of expressions.


So the best solution is to not have any suffix on any of the time 
constants since none of them needs it. That's what I have proposed.

The next best solution is to have all of them be int64_t since that's 
the type of the variables they are usually (but not necessarily) used 
with. That includes NSEC_PER_MICROSEC and MSEC_PER_SEC. This violates 
the second point though.

In a distant third place is adding the LL suffix to all constants or 
casting them all to some long long type. It's not as good as the 
solution above since we essentially don't use long long variables 
anywhere (I count a total of three long long variables in some dark 
corner of spice-common) and thus will catch developers off-guard when 
they try to trace their values.

And the worst option is the current situation where half the constants 
force an unused type on all expressions where they are used in and half 
don't.


On Tue, 25 Jun 2019, Frediano Ziglio wrote:
[...]
> > Whenever NSEC_PER_MILLISEC or NSEC_PER_SEC are used in a spice_debug()
> > parameter one cannot use %u or %lu as the format. But not so for
> 
> Being signed you would use %lld or similars.

As the code currently stands that's only true for expressions that use 
NSEC_PER_SEC. All expressions that use the other time constants have the 
type of the variables used in the expression which means one should use 
either %d/%u or %ld/%lu.


> > NSEC_PER_MICROSEC or MSEC_PER_SEC. This is inconsistent so that timing
> 
> No, you cannot use long or not long for other constants too, they are
> different for 32-bit so with both you cannot mix.

???


> > traces require lots of guessing and recompilations.
> > 
> 
> That's why we use -Wformat so compiler will tell you which ones are wrong.
> I don't see why you need to guess and recompile,
> beside I suppose the first time you are writing the code.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Precisely. And since it's used in one-off debugging expressions it keeps 
coming up. Plus half the spice_debug() arguments are hidden so that when 
the compiler says there's a problem with argument 5 it's anybody's guess 
as to where the problem actually lies.

Sure all is well if you memorize that NSEC_PER_SEC and NSEC_PER_MILLISEC 
are long longs, but NSEC_PER_MILLISEC is not anymore, and neither are 
any of the other time constants and that spice_debug() has precisely x 
hidden arguments which you must substract from compiler error messages.

But that's just setting up traps for developers who I argue have much 
better things to memorize and think about.


> And if you need to guess it means you don't know the types you are 
> using and so you are not sure about overflows so you are not paying 
> much attention to the code you are writing.

I know what the types of the variables I'm using are, thank you. It's 
the constants that have inconsistent and confusing types and change the 
type of the expression I'm using them with.

Also, with C's implicit casts the LL suffix is not any better at 
avoiding overflows than having an int64_t cast backed into the 
constants.

It's only when using the constants are used as arguments to a 
printf()-equivalent that the LL trap is sprung. And I'd argue this is 
not a case where there's much of an overflow risk to start with, and not 
one where it would even matter much since most of those are temporary 
one-off debugging traces.


-- 
Francois Gouget <fgouget at codeweavers.com>