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

[Francois Gouget]

On Tue, 25 Jun 2019, Frediano Ziglio wrote:
[...]
> >     uint64_t foo = 1234;
> >     spice_debug("foo=%" PRId64, foo / NSEC_PER_SEC);
[...]
> If you assume long long == 64 bit should not be a big problem
> although you can still have the warning.

Not a warning. A compilation error:

  CC       gstreamer-encoder.lo
In file included from red-common.h:28,
                 from gstreamer-encoder.c:30:
gstreamer-encoder.c: In function 'get_min_playback_delay':
../subprojects/spice-common/common/log.h:67:62: error: format '%ld' 
expects argument of type 'long int', but argument 5 has type 'long long unsigned int' [-Werror=format=]
     spice_log(G_LOG_LEVEL_DEBUG, SPICE_STRLOC, __FUNCTION__, "" format, ## __VA_ARGS__); \
                                                              ^~
gstreamer-encoder.c:606:5: note: in expansion of macro 'spice_debug'
     spice_debug("foo=%" PRId64, foo / NSEC_PER_SEC);
     ^~~~~~~~~~~
In file included from gstreamer-encoder.c:21:
/usr/include/inttypes.h:57:34: note: format string is defined here
 # define PRId64  __PRI64_PREFIX "d"
cc1: all warnings being treated as errors


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

And saying that means you consider the principle of least surprise to be 
completely useless.


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

I can make an exception for MSEC_PER_SEC.


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

I'd very much prefer the cast to be in the expression rather than hidden 
in some far away macro.

For instance:

    int frames_count;
    ...

    if (duration > NSEC_PER_SEC * drawable->frames_count / MAX_FPS) {

Anyone checking this calculation would think that there is a risk of 
overflow. And it's only when tracing NSEC_PER_SEC to utils.h that they 
would discover that NSEC_PER_SEC forces 64 bit calculation.

At least this form is clear right away:

    if (duration > ((uint64_t)NSEC_PER_SEC) * drawable->frames_count / MAX_FPS) {


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

You mean in case one compiles to 32 bit?
Using PRId64 and PRIu64 would still not work with NSEC_PER_SEC which is 
the issue here.



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

No. If I want to trace a 64 bit variable I just use %ld/%lu and that's 
it. No need to look at utils.h to refresh my memory on whether a 
specific macro is a long long unlike all the other similarly named ones.


> And this patch is not fixing these macros.

It removes the LL suffix from NSEC_PER_SEC, thus making it consistent 
with all other NSEC_PER_XXX macros and even MSEC_PER_SEC. So yes it 
does. Or where you speaking of some other macros?

After this patch the only constants that keep a 64 bit type are those 
that don't fit in 32 bit, and at least those are longs, not long longs.


-- 
Francois Gouget <fgouget at codeweavers.com>