[Spice-devel] [PATCH spice-server] display-channel: Check that all structure are destroyed during finalize

[Frediano Ziglio]

> 
> On Thu, Aug 24, 2017 at 05:11:34AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Tue, 2017-08-22 at 08:54 +0100, Frediano Ziglio wrote:
> > > > The leak detector we use currently is not enough to detect
> > > > some kind of leak in DisplayChannel so manully test.
> > > > These tests are enabled only when --enable-extra-checks is passed
> > > > to configure.
> > > 
> > > 
> > > Out of curiosity, did you find a bug that made you add this extra
> > > check?
> > > 
> > 
> > No, was more some old code I wrote (I think more than 1 year ago)
> > when I started my "cleanup" branch.
> > 
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > >  configure.ac             |  2 ++
> > > >  server/display-channel.c | 22 ++++++++++++++++++++++
> > > >  2 files changed, 24 insertions(+)
> > > > 
> > > > diff --git a/configure.ac b/configure.ac
> > > > index e1e74862..7fec66fc 100644
> > > > --- a/configure.ac
> > > > +++ b/configure.ac
> > > > @@ -247,6 +247,8 @@ 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])
> > 
> > A bit of note on this. Usually AC_DEFINEs are not defined or defined
> > to 1. This is defined to 0 or 1 which is a bit different from the
> > standard. However code like:
> > 
> >    if (ENABLE_EXTRA_CHECKS)
> > 
> > cause a compiler error. Maybe the comment "Define to 1 to enable
> > extra checks on code" should be more explicit about it?
> > On the other way this should affect only people that needs
> > to edit manually config.h and as said would be generating a
> > compiler error if they undefine that constant.
> > A difference from the standard is that code like
> > 
> > #ifdef ENABLE_EXTRA_CHECKS
> > 
> > should not be used. Maybe a syntax-check extension could help
> > with this.
> 
> I would have gone with something more standard,
> 
> if test "x$enable_extra_checks" = xyes; then
>   AC_DEFINE_UNQUOTED([ENABLE_EXTRA_CHECKS],
>                      [1],
>                      [Define to 1 to enable extra checks on code otherwise
>                      defined to 0])
> fi
> 
> and then
> 
> #ifdef ENABLE_EXTRA_CHECKS
> static int enable_extra_checks = 1;
> #else
> static int enable_extra_checks = 0;
> #endif
> 
> if (enable_extra_checks) {
> }
> 
> Christophe
> 

Does not work, "static int" are not compile time constant.
And as you realized not even "static const int" in C
(they are in C++).

#defines and enums are compile time constants so a

#ifdef ENABLE_EXTRA_CHECKS
enum { enable_extra_checks = 1 };
#else
enum { enable_extra_checks = 0 };
#endif

would work, or a

#ifndef ENABLE_EXTRA_CHECKS
#define ENABLE_EXTRA_CHECKS 0
#endif

OT: why if DEFINE is not defined as a preprocessor macro
"#if DEFINE" is false?

Frediano