[Spice-devel] [spice-common v2 1/6] quic: Use DEFevol constant rather than evol variable

[Christophe Fergeau]

On Tue, Aug 01, 2017 at 10:47:23AM -0400, Frediano Ziglio wrote:
> > 
> > We never change 'evol' value, and this is currently causing issues with
> > gcc 7.1.1: quic.c is checking at compile-time that 'evol' is 1, 3 or 5.
> > This is a constant, so a static check should be good, but the compiler (gcc
> > 7.1.1) is unable to know 'evol' value at compile-time. Since the removal
> > of spice_static_assert in favour of SPICE_VERIFY, this causes a
> > compile-time failure.
> > 
> > Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> > ---
> >  common/quic.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> > 
> > diff --git a/common/quic.c b/common/quic.c
> > index c188ed2..1deda47 100644
> > --- a/common/quic.c
> > +++ b/common/quic.c
> > @@ -178,9 +178,6 @@ static const int wmimax = DEFwmimax;
> >  /* number of symbols to encode before increasing wait mask index */
> >  static const int wminext = DEFwminext;
> >  
> > -/* model evolution mode */
> > -static const int evol = DEFevol;
> > -
> >  /* bppmask[i] contains i ones as lsb-s */
> >  static const unsigned long int bppmask[33] = {
> >      0x00000000, /* [0] */
> > @@ -248,14 +245,14 @@ static unsigned int tabrand(unsigned int *tabrand_seed)
> >      return tabrand_chaos[++*tabrand_seed & TABRAND_SEEDMASK];
> >  }
> >  
> > -static const unsigned short besttrigtab[3][11] = { /* array of wm_trigger
> > for waitmask and evol,
> > +static const unsigned short besttrigtab[3][11] = { /* array of wm_trigger
> > for waitmask and DEFevol,
> >                                                      used by set_wm_trigger()
> >                                                      */
> >      /* 1 */ { 550, 900, 800, 700, 500, 350, 300, 200, 180, 180, 160},
> >      /* 3 */ { 110, 550, 900, 800, 550, 400, 350, 250, 140, 160, 140},
> >      /* 5 */ { 100, 120, 550, 900, 700, 500, 400, 300, 220, 250, 160}
> >  };
> >  
> > -/* set wm_trigger knowing waitmask (param) and evol (glob)*/
> > +/* set wm_trigger knowing waitmask (param) and DEFevol (glob)*/
> >  static void set_wm_trigger(CommonState *state)
> >  {
> >      unsigned int wm = state->wmidx;
> > @@ -263,9 +260,9 @@ static void set_wm_trigger(CommonState *state)
> >          wm = 10;
> >      }
> >  
> > -    spice_assert(evol < 6);
> > +    spice_assert(DEFevol < 6);
> >  
> > -    state->wm_trigger = besttrigtab[evol / 2][wm];
> > +    state->wm_trigger = besttrigtab[DEFevol / 2][wm];
> >  
> >      spice_assert(state->wm_trigger <= 2000);
> >      spice_assert(state->wm_trigger >= 1);
> > @@ -878,7 +875,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_VERIFY(evol == 1 || evol == 3 || evol == 5);
> > +    SPICE_VERIFY(DEFevol == 1 || DEFevol == 3 || DEFevol == 5);
> >      spice_assert(bpc <= 8 && bpc > 0);
> >  
> >      *ncounters = 8;
> > @@ -887,7 +884,7 @@ static void find_model_params(Encoder *encoder,
> >  
> >      *n_buckets_ptrs = 0;  /* ==0 means: not set yet */
> >  
> > -    switch (evol) {   /* set repfirst firstsize repnext mulsize */
> > +    switch (DEFevol) {   /* set repfirst firstsize repnext mulsize */
> 
> As we decided to not change this parameter... should we remove the switch?

There are a few places where the code could be simplified, yes, here and
also the 'besttrigtab' array could be single dimension.

> 
> >      case 1: /* buckets contain following numbers of contexts: 1 1 1 2 2 4 4
> >      8 8 ... */
> >          *repfirst = 3;
> >          *firstsize = 1;
> > @@ -907,7 +904,7 @@ static void find_model_params(Encoder *encoder,
> >          *mulsize = 4;
> >          break;
> >      default:
> > -        encoder->usr->error(encoder->usr, "findmodelparams(): evol out of
> > range!!!\n");
> > +        encoder->usr->error(encoder->usr, "findmodelparams(): DEFevol out of
> > range!!!\n");
> >          return;
> >      }
> >
> 
> I checked and executable code does not change.
> Beside the comment I would say
> 
> Acked-by: Frediano Ziglio <fziglio at redhat.com>

Thanks, pushed.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170802/6c99cf62/attachment.sig>