[Spice-devel] [spice-server] build-sys: Improve ENABLE_EXTRA_CHECK setting

[Frediano Ziglio]

> 
> On Tue, Sep 12, 2017 at 06:20:55AM -0400, Frediano Ziglio wrote:
> > >
> > > On Tue, Sep 12, 2017 at 06:06:29AM -0400, Frediano Ziglio wrote:
> > > > > 
> > > > > Currently, ENABLE_EXTRA_CHECK is defined to be 0 or 1, and has to be
> > > > > defined this way, otherwise this will break display-channel.c
> > > > > compilation.
> > > > > This is different from most AC_DEFINE() preprocessor constants which
> > > > > are
> > > > > usually either set to 1, or unset.
> > > > > 
> > > > > This commit switches ENABLE_EXTRA_CHECK setting in configure.ac to
> > > > > the
> > > > > usual way, and defines a second constant to 0/1 depending on whether
> > > > > it's set or not in display-channel.c.
> > > > > 
> > > > > Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> > > > > ---
> > > > >  configure.ac             | 6 +++---
> > > > >  server/display-channel.c | 8 +++++++-
> > > > >  2 files changed, 10 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/configure.ac b/configure.ac
> > > > > index 483dbfdf8..c0ea12b5a 100644
> > > > > --- a/configure.ac
> > > > > +++ b/configure.ac
> > > > > @@ -237,9 +237,9 @@ 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])
> > > > > +AS_IF([test "x$enable_extra_checks" = xyes],
> > > > > +      AC_DEFINE([ENABLE_EXTRA_CHECKS], [1],
> > > > > +                [Define to 1 to enable extra checks on code
> > > > > otherwise
> > > > > define
> > > > > to 0]))
> > > > 
> > > > comment should be updated
> > > > 
> > > > >  
> > > > >  dnl
> > > > >  ===========================================================================
> > > > >  dnl check compiler flags
> > > > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > > > index f7e36dbb4..5cfc151cd 100644
> > > > > --- a/server/display-channel.c
> > > > > +++ b/server/display-channel.c
> > > > > @@ -80,6 +80,12 @@ display_channel_set_property(GObject *object,
> > > > >      }
> > > > >  }
> > > > >  
> > > > > +#ifdef HAVE_EXTRA_CHECKS
> > > > > +#define EXTRA_CHECKS_ENABLED 1
> > > > > +#else
> > > > > +#define EXTRA_CHECKS_ENABLED 0
> > > > > +#endif
> > > > > +
> > > > 
> > > > 
> > > > This should be defined in a more global header to get reused.
> > > 
> > > At the moment this is only used in one place, if this gets reused, it
> > > will be a good time to move it.
> > > 
> > 
> > Can't you design for the future?
> 
> Maybe this will get used more in the future, maybe not. It's a 10
> seconds job to move it somewhere else, so why pollute the global
> namespace now when we have no idea whether this is going to get used
> more or not?
> 
> Christophe
> 

In this case you are introducing a regression, current code is
designed to support this already. I cannot surely say that this
patch is an improvement.

Frediano