On 19/03/18 06:28, Frediano Ziglio wrote: >> >> On 15/03/18 14:20, Christophe Fergeau wrote: >>> On Tue, Mar 13, 2018 at 10:37:46AM -0300, Eduardo Lima (Etrunko) wrote: >>>> On 13/03/18 04:21, Frediano Ziglio wrote: >>>>>> >>>>>> This patch makes it clear that this is a configure switch and not a >>>>>> variable defined somewhere else in the code. >>>>>> >>>>> >>>>> The code is intended that way to make the compiler always parse >>>>> these parts. Note that that define is always defined so your code >>>>> is not doing what you are intending. >>>> >>>> I have sent this patch by mistake, but anyway, the fact of it always >>>> being defined is true with autotools, but it is not with meson. >>>> >>>> Do you think it would make sense to have this patch or is it better to >>>> keep as is? If the latter, I think it would be better to keep a static >>>> variable and change its value according to the define. >>> >>> For what it's worth, I tried doing something like what you suggest in >>> the past >>> https://lists.freedesktop.org/archives/spice-devel/2017-September/039963.html >>> but I came to the conclusion that this was not going to work nicely >>> https://lists.freedesktop.org/archives/spice-devel/2017-September/039983.html >>> I don't fully recall what the problem(s) were though :( >>> >> >> Looks like I missed that discussion, but as there are very few places in >> the code that make use of this define, we could make it a static global >> to each file that makes the use. >> >> #ifdef ENABLE_EXTRA_CHECKS >> static const int extra_checks = 1; >> #else >> static const int extra_checks = 0; >> #endif >> >> And then replace accordingly. >> > > What about something like this on spice-common: > > > diff --git a/common/log.h b/common/log.h > index 06d48d2..f9e1245 100644 > --- a/common/log.h > +++ b/common/log.h > @@ -93,6 +93,12 @@ void spice_log(GLogLevelFlags log_level, > } \ > } G_STMT_END > > +#if ENABLE_EXTRA_CHECKS > +enum { extra_checks = 1 }; > +#else > +enum { extra_checks = 0 }; > +#endif > + > SPICE_END_DECLS > > #endif /* H_SPICE_LOG */ > diff --git a/configure.ac b/configure.ac > index 3542161..3da85de 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -27,6 +27,7 @@ fi > AM_PROG_CC_C_O > > SPICE_CHECK_SYSDEPS > +SPICE_EXTRA_CHECKS > > AC_ARG_ENABLE([alignment-checks], > AS_HELP_STRING([--enable-alignment-checks], > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4 > index 68e3091..b969c4f 100644 > --- a/m4/spice-deps.m4 > +++ b/m4/spice-deps.m4 > @@ -22,6 +22,14 @@ AC_DEFUN([SPICE_PRINT_MESSAGES],[ > IFS="$ac_save_IFS" > ]) > > +AC_DEFUN([SPICE_EXTRA_CHECKS],[ > +AC_ARG_ENABLE([extra-checks], > + AS_HELP_STRING([--enable-extra-checks=@<:@yes/no@:>@], > + [Enable expensive checks @<:@default=no@:>@])) > +AM_CONDITIONAL(ENABLE_EXTRA_CHECKS, test "x$enable_extra_checks" = "xyes") > +AS_IF([test "x$enable_extra_checks" = "xyes"], > + [AC_DEFINE([ENABLE_EXTRA_CHECKS], 1, [Enable extra checks on code])]) > +]) > > # SPICE_CHECK_SYSDEPS() > # --------------------- > > > and in spice-server (and possibly spice-gtk too for the configure.ac): > > > diff --git a/configure.ac b/configure.ac > index 86383434..2443ccf3 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -71,6 +71,7 @@ esac > dnl ========================================================================= > dnl Check optional features > SPICE_CHECK_SMARTCARD > +SPICE_EXTRA_CHECKS > > AC_ARG_ENABLE(gstreamer, > AS_HELP_STRING([--enable-gstreamer=@<:@auto/0.10/1.0/yes/no@:>@], > @@ -237,14 +238,6 @@ AC_ARG_ENABLE([statistics], > AS_IF([test "$enable_statistics" = "yes"], > [AC_DEFINE([RED_STATISTICS], [1], [Enable SPICE statistics])]) > > -AC_ARG_ENABLE([extra-checks], > - AS_HELP_STRING([--enable-extra-checks=@<:@yes/no@:>@], > - [Enable expensive checks @<:@default=no@:>@])) > -AM_CONDITIONAL(ENABLE_EXTRA_CHECKS, test "$enable_extra_checks" = "yes") > -AC_DEFINE_UNQUOTED([ENABLE_EXTRA_CHECKS], > - [$(test "x$enable_extra_checks" = xyes && echo 1 || echo 0)], > - [Define to 1 to enable extra checks on code otherwise define to 0]) > - > dnl =========================================================================== > dnl check compiler flags > > diff --git a/server/display-channel.c b/server/display-channel.c > index 6dc10ee7..6511419d 100644 > --- a/server/display-channel.c > +++ b/server/display-channel.c > @@ -89,7 +89,7 @@ display_channel_finalize(GObject *object) > display_channel_destroy_surfaces(self); > image_cache_reset(&self->priv->image_cache); > > - if (ENABLE_EXTRA_CHECKS) { > + if (extra_checks) { > unsigned int count; > _Drawable *drawable; > VideoStream *stream; > > > > - the usage of the enum make sure extra_checks is a compile time constant; > - the macro/constant can be used also in spice-common; > - the --enable-extra-checks is shared between different projects; > - ENABLE_EXTRA_CHECKS is defined as 1 or undefined which is more standard; > - I think common/log.h is a good place to define that constant as is often > used with spice_assert. > > Frediano > I like the idea of the enum, it looks good to me, although I would add 'spice_' or 'spice_common_' prefix to the name, now that it has got a broader scope. -- Eduardo de Barros Lima (Etrunko) Software Engineer - RedHat etrunko@xxxxxxxxxx _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel