Re: [Spice-devel] [PATCH spice-common 1/2] quic: constantify some variable

[Marc-André Lureau]

On Mon, Jun 12, 2017 at 11:21 AM Frediano Ziglio  wrote:

> Signed-off-by: Frediano Ziglio 
>

Reviewed-by: Marc-André Lureau 



> ---
>  common/quic.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/common/quic.c b/common/quic.c
> index 5b00d65..59565ae 100644
> --- a/common/quic.c
> +++ b/common/quic.c
> @@ -173,13 +173,13 @@ struct Encoder {
>  };
>
>  /* target wait mask index */
> -static int wmimax = DEFwmimax;
> +static const int wmimax = DEFwmimax;
>
>  /* number of symbols to encode before increasing wait mask index */
> -static int wminext = DEFwminext;
> +static const int wminext = DEFwminext;
>
>  /* model evolution mode */
> -static int evol = DEFevol;
> +static const int evol = DEFevol;
>
>  /* bppmask[i] contains i ones as lsb-s */
>  static const unsigned long int bppmask[33] = {
> --
> 2.9.4
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common 1/2] quic: constantify some variable

[Frediano Ziglio]

> 
> Hi
> 
> - Original Message -
> > > 
> > > - Original Message -
> > > > Signed-off-by: Frediano Ziglio 
> > > > ---
> > > >  common/quic.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/common/quic.c b/common/quic.c
> > > > index 5b00d65..59565ae 100644
> > > > --- a/common/quic.c
> > > > +++ b/common/quic.c
> > > > @@ -173,13 +173,13 @@ struct Encoder {
> > > >  };
> > > >  
> > > >  /* target wait mask index */
> > > > -static int wmimax = DEFwmimax;
> > > > +static const int wmimax = DEFwmimax;
> > > >  
> > > >  /* number of symbols to encode before increasing wait mask index */
> > > > -static int wminext = DEFwminext;
> > > > +static const int wminext = DEFwminext;
> > > >  
> > > >  /* model evolution mode */
> > > > -static int evol = DEFevol;
> > > > +static const int evol = DEFevol;
> > > >  
> > > >  /* bppmask[i] contains i ones as lsb-s */
> > > >  static const unsigned long int bppmask[33] = {
> > > 
> > > 
> > > Ah right, you fixed it here. I wonder if we should stick to a upper case
> > > convention for const global values. And I wonder if it shouldn't be a
> > > #define instead.
> > > 
> > 
> > Usually for compatibility I use defines but I don't think is
> > a big problem for the compilers we use. A static const should be
> > a compile time constant (OT: a non static one is not so in C
> > but so in C++).
> > 
> 
> I am not thinking about compile-time optimization, but rather making clear
> that the value can't be tweaked (even with a debugger) by using upper-case
> macro convention.
> > Frediano
> > 
> 

A compile time constant and a define requires the same kind of tweaks
from a debugger prospective. The only difference from a debugger is
that with the define macro you don't see the mnemonic.
As a developer if I read "static const" I assume is a constant
(I don't really understand your confusion here). And on the usage
occurrences (like "switch (evol)") the compiler won't allow me to change
the constant. I think this code was written that way to allow to change
(maybe even at runtime) these variable so it made sense.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common 1/2] quic: constantify some variable

[Marc-André Lureau]

Hi

- Original Message -
> > 
> > - Original Message -
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  common/quic.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/common/quic.c b/common/quic.c
> > > index 5b00d65..59565ae 100644
> > > --- a/common/quic.c
> > > +++ b/common/quic.c
> > > @@ -173,13 +173,13 @@ struct Encoder {
> > >  };
> > >  
> > >  /* target wait mask index */
> > > -static int wmimax = DEFwmimax;
> > > +static const int wmimax = DEFwmimax;
> > >  
> > >  /* number of symbols to encode before increasing wait mask index */
> > > -static int wminext = DEFwminext;
> > > +static const int wminext = DEFwminext;
> > >  
> > >  /* model evolution mode */
> > > -static int evol = DEFevol;
> > > +static const int evol = DEFevol;
> > >  
> > >  /* bppmask[i] contains i ones as lsb-s */
> > >  static const unsigned long int bppmask[33] = {
> > 
> > 
> > Ah right, you fixed it here. I wonder if we should stick to a upper case
> > convention for const global values. And I wonder if it shouldn't be a
> > #define instead.
> > 
> 
> Usually for compatibility I use defines but I don't think is
> a big problem for the compilers we use. A static const should be
> a compile time constant (OT: a non static one is not so in C
> but so in C++).
> 

I am not thinking about compile-time optimization, but rather making clear that 
the value can't be tweaked (even with a debugger) by using upper-case macro 
convention. 
> Frediano
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common 1/2] quic: constantify some variable

[Frediano Ziglio]

> 
> - Original Message -
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  common/quic.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/common/quic.c b/common/quic.c
> > index 5b00d65..59565ae 100644
> > --- a/common/quic.c
> > +++ b/common/quic.c
> > @@ -173,13 +173,13 @@ struct Encoder {
> >  };
> >  
> >  /* target wait mask index */
> > -static int wmimax = DEFwmimax;
> > +static const int wmimax = DEFwmimax;
> >  
> >  /* number of symbols to encode before increasing wait mask index */
> > -static int wminext = DEFwminext;
> > +static const int wminext = DEFwminext;
> >  
> >  /* model evolution mode */
> > -static int evol = DEFevol;
> > +static const int evol = DEFevol;
> >  
> >  /* bppmask[i] contains i ones as lsb-s */
> >  static const unsigned long int bppmask[33] = {
> 
> 
> Ah right, you fixed it here. I wonder if we should stick to a upper case
> convention for const global values. And I wonder if it shouldn't be a
> #define instead.
> 

Usually for compatibility I use defines but I don't think is
a big problem for the compilers we use. A static const should be
a compile time constant (OT: a non static one is not so in C
but so in C++).

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common 1/2] quic: constantify some variable

[Marc-André Lureau]

- Original Message -
> Signed-off-by: Frediano Ziglio 
> ---
>  common/quic.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/common/quic.c b/common/quic.c
> index 5b00d65..59565ae 100644
> --- a/common/quic.c
> +++ b/common/quic.c
> @@ -173,13 +173,13 @@ struct Encoder {
>  };
>  
>  /* target wait mask index */
> -static int wmimax = DEFwmimax;
> +static const int wmimax = DEFwmimax;
>  
>  /* number of symbols to encode before increasing wait mask index */
> -static int wminext = DEFwminext;
> +static const int wminext = DEFwminext;
>  
>  /* model evolution mode */
> -static int evol = DEFevol;
> +static const int evol = DEFevol;
>  
>  /* bppmask[i] contains i ones as lsb-s */
>  static const unsigned long int bppmask[33] = {


Ah right, you fixed it here. I wonder if we should stick to a upper case 
convention for const global values. And I wonder if it shouldn't be a #define 
instead.


> --
> 2.9.4
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-common 1/2] quic: constantify some variable

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 common/quic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/common/quic.c b/common/quic.c
index 5b00d65..59565ae 100644
--- a/common/quic.c
+++ b/common/quic.c
@@ -173,13 +173,13 @@ struct Encoder {
 };
 
 /* target wait mask index */
-static int wmimax = DEFwmimax;
+static const int wmimax = DEFwmimax;
 
 /* number of symbols to encode before increasing wait mask index */
-static int wminext = DEFwminext;
+static const int wminext = DEFwminext;
 
 /* model evolution mode */
-static int evol = DEFevol;
+static const int evol = DEFevol;
 
 /* bppmask[i] contains i ones as lsb-s */
 static const unsigned long int bppmask[33] = {
-- 
2.9.4

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel