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

[Frediano Ziglio]

> 
> 
> 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);
> 

If you assume long long == 64 bit should not be a big problem
although you can still have the warning.
But I don't think Uri was thinking at this case.
One solution for this would be to use INT64_C macro instead of
the LL suffix.

> These are traps. It's on the same level as:
> 
> #define ONE 2
> 
> Useless inconsistencies and misdirections.
> 

Time types are not really consistent, you are expected to have
different types and precisions, I don't see the extreme case
like the "ONE".

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

Surprise/astonishment are really subject to human opinion. They are based
on habits and expectations. Saying that "principle of least surprise"
dictates something is pretending all people have the same habits/expectations.

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

It sounds reasonable, all these constants are defined really closed in
the source and express the same precision (nanoseconds). I don't expect
MSEC_PER_SEC to have the same precision/type.

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

C++ added an extension to make enumeration typed, C has some extension
to achieve that and compilers have options for this so it seems in
different cases it's wanted.

> 
> 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.
> 

I don't agree on MSEC_PER_SEC and second point so it seems reasonable
to me as solution too.

> 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.
> 

Yes, I think all NSEC_PER_* should have the same type.

> 
> 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.
> 

Most are 64 bit so all %d, %u, %ld and %lu are not good.

> 
> > > 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.
> 

But most of types are 64 bit so you have the same issue too, isn't it?
And this patch is not fixing these macros.

> 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

My sentence does not contain any "variable" in it.

> 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.
> 

I agree.

> 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.
> 
> 

I suppose either this patch or having all NSEC_PER_SEC/NSEC_PER_MILLISEC/
NSEC_PER_MICROSEC wrapped in INT64_C sounds reasonable to me.

Frediano