[Spice-devel] [PATCH spice-server 1/2] Change ENABLE_EXTRA_CHECKS statements to #ifdef

[Eduardo Lima (Etrunko)]

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 at redhat.com