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

[Frediano Ziglio]

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

No, this is a warning treated like an error.
But I agree that as this could cause memory errors should be fixed, that
is is a warning to be better treated as an error.

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

So if a Python developer found that numbers with overflows are astonishing
we should move to Python? What I'm saying is that using a principle based
on opinions and strong inference is not correct. It's just that is not Math.

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

Long is not enough, it does not cover 32 bit or LLP64, all platforms we
support.
I think all (people in this thread) agreed on not using "long long" but
int64_t (either cast or specific constant modifier) or int (no modifier).

I personally agree that NSEC_PER_* should all have the same type. Not sure
about what Uri thinks about this.

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

I was speaking about spice_debug and similar.

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

I'm not strong about this patch or all int64_t for NSEC_PER_*.
That counts to me as 1.5 votes against 1.5.

Frediano