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@xxxxxxxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel