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