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

[Frediano Ziglio]

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