[Spice-devel] [PATCH spice-common 2/2] log: remove spice_static_assert macro

[Frediano Ziglio]

> 
> Hi
> 
> ----- Original Message -----
> > > 
> > > Hi
> > > 
> > > ----- Original Message -----
> > > > The macro was misused and not doing static check.
> > > > Spice have other working static check macros to use.
> > > > The macro is used only by spice-common so removing it
> > > > does not cause issues to other depending projects.
> > > > 
> > > 
> > > See also "[PATCH spice-common 1/4] log: replace spice log with glib"
> > > 
> > 
> > Still to have a better look but looks like comparing a mouse
> > with an elephant.
> 
> How so? we could get rid of a lot of duplicated and unmaintained code by
> using glib equivalent.
> 

In this case we are using GNUlib macros (SPICE_VERIFY is just a small
wrapper). Considering we'll have to use GLib and fix their macro and
that we are already reusing code for this specific case I prefer to
stick with current macros.

> > 
> > > I think we should replace SPICE_VERIFY / verify with G_STATIC*
> > > equivalents.
> > >  
> > 
> > We actually can't for compatibility reason. Some version of GLib have
> > broken macros and we still claim RHEL 6 support
> > 
> 
> Ah, I vaguely recall some discussion, do you have a pointer? I can't find
> changes in glib git log.
> 

I think Christophe F pointed out the problem. Don't remember exactly, I
think they were using array sizes but this do not work with C99.
Looking at current GLib master code is still using that broken
trick.

> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > >  common/log.h              | 5 -----
> > > >  common/quic.c             | 2 +-
> > > >  common/quic_family_tmpl.c | 2 +-
> > > >  3 files changed, 2 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/common/log.h b/common/log.h
> > > > index 1ea2313..a4d296d 100644
> > > > --- a/common/log.h
> > > > +++ b/common/log.h
> > > > @@ -94,11 +94,6 @@ void spice_log(const char *log_domain,
> > > >      }                                                   \
> > > >  } G_STMT_END
> > > >  
> > > > -/* FIXME: improve that some day.. */
> > > > -#define spice_static_assert(x) SPICE_STMT_START {       \
> > > > -    spice_assert(x);                                    \
> > > > -} SPICE_STMT_END
> > > > -
> > > >  SPICE_END_DECLS
> > > >  
> > > >  #endif /* H_SPICE_LOG */
> > > > diff --git a/common/quic.c b/common/quic.c
> > > > index 59565ae..2cd1f0d 100644
> > > > --- a/common/quic.c
> > > > +++ b/common/quic.c
> > > > @@ -888,7 +888,7 @@ static void find_model_params(Encoder *encoder,
> > > >      /* The only valid values are 1, 3 and 5.
> > > >         0, 2 and 4 are obsolete and the rest of the
> > > >         values are considered out of the range. */
> > > > -    spice_static_assert (evol == 1 || evol == 3 || evol == 5);
> > > > +    SPICE_VERIFY(evol == 1 || evol == 3 || evol == 5);
> > > 
> > > Hmm, is that really static?
> > > static int evol = DEFevol;
> > > 
> > > Could be eventually modified at run time..
> > > 
> > > >      spice_assert(bpc <= 8 && bpc > 0);
> > > >  
> > > >      *ncounters = 8;
> > > > diff --git a/common/quic_family_tmpl.c b/common/quic_family_tmpl.c
> > > > index 9a434e0..9450f44 100644
> > > > --- a/common/quic_family_tmpl.c
> > > > +++ b/common/quic_family_tmpl.c
> > > > @@ -71,7 +71,7 @@ static unsigned int FNAME(golomb_decoding)(const
> > > > unsigned
> > > > int l, const unsigned
> > > >  static void FNAME(update_model)(CommonState *state, s_bucket * const
> > > >  bucket,
> > > >                                  const BYTE curval)
> > > >  {
> > > > -    spice_static_assert(BPC >= 1);
> > > > +    SPICE_VERIFY(BPC >= 1);
> > > >      spice_return_if_fail (bucket != NULL);
> > > >  
> > > >      const unsigned int bpp = BPC;
> > 

Frediano