[Spice-devel] [PATCH 2/2] Define and use new SPICE_MAGIC_CONST macro

[Frediano Ziglio]

> 
> On Fri, Aug 07, 2015 at 01:29:57PM +0100, Frediano Ziglio wrote:
> > This macro allow to define magic constants without using weird
> > memory tweacks.
> > This remove some possible warning from some compiler and
> > make code more optimized as compiler is able to compute the
> > constant.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  spice/controller_prot.h   | 2 +-
> >  spice/foreign_menu_prot.h | 2 +-
> >  spice/macros.h            | 6 ++++++
> >  spice/protocol.h          | 2 +-
> >  spice/qxl_dev.h           | 4 ++--
> >  spice/stats.h             | 2 +-
> >  spice/vdi_dev.h           | 2 +-
> >  7 files changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/spice/macros.h b/spice/macros.h
> > index 018ff32..4647ee3 100644
> > --- a/spice/macros.h
> > +++ b/spice/macros.h
> > @@ -359,5 +359,11 @@
> >  #  define SPICE_BYTESWAP64(val) (SPICE_BYTESWAP64_CONSTANT (val))
> >  #endif /* generic */
> >  
> > +#if SPICE_ENDIAN == SPICE_ENDIAN_LITTLE
> > +#define SPICE_MAGIC_CONST(s)
> > ((uint32_t)(s[0]+s[1]*0x100u+s[2]*0x10000u+s[3]*0x1000000u))
> > +#else
> > +#define SPICE_MAGIC_CONST(s)
> > ((uint32_t)(s[3]+s[2]*0x100u+s[1]*0x10000u+s[0]*0x1000000u))
> > +#endif
> > +
> 
> If I'm not mistaken, we can keep the (*(const uint32_t*)"QXRO") method
> for little-endian (or are you avoiding -Wcast-align warnings at
> the same time)? This could potentially use SPICE_BYTESWAP32_CONSTANT
> (but I assume there is still this -Wcast-align warning to take care of?)
> What about SPICE_ENDIAN_PDP, I believe this is not going to be correct?
> (I would just error out at compile time if we find out the endianness is
> PDP). I'd favour << and | over +/* for what it's worth, and make sure we
> are operating on unsigned chars.
> 
> Christophe
> 

The (*(const uint32_t*)"QXRO") does not produce a constant (at least on gcc)
but a string and a pointer dereference.
I don't cast to unsigned char as actually all strings are ASCII (so cast is
not necessary). The * instead of << make the code smaller as doing a
((char) c) << 24 just can lead to 0 in all cases while ((char) c) * 0x1000000u
force the cast char -> unsigned.
There is already the check for SPICE_ENDIAN_PDP.
Yes, I can use SPICE_BYTESWAP32_CONSTANT (it just expands in a really long code).
A code like this can work

#if SPICE_ENDIAN == SPICE_ENDIAN_LITTLE
#define SPICE_MAGIC_CONST(s) \
 ((uint32_t)((s[0]&0xffu)|((s[1]&0xffu)<<8)|((s[2]&0xff)<<16)|((s[3]&0xffu)<<24))
#else
#define SPICE_MAGIC_CONST(s) \
 ((uint32_t)((s[3]&0xffu)|((s[2]&0xffu)<<8)|((s[1]&0xff)<<16)|((s[0]&0xffu)<<24))
#endif

or

#if SPICE_ENDIAN == SPICE_ENDIAN_LITTLE
#define SPICE_MAGIC_CONST(s) \
 ((uint32_t)((s[0]&0xffu)|((s[1]&0xffu)<<8)|((s[2]&0xff)<<16)|((s[3]&0xffu)<<24))
#else
#define SPICE_MAGIC_CONST(s) \
 SPICE_BYTESWAP32_CONSTANT((s[0]&0xffu)|((s[1]&0xffu)<<8)|((s[2]&0xff)<<16)|((s[3]&0xffu)<<24))
#endif


Frediano