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