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