Re: [PATCH] ui: drop VNC feature _MASK constants

[Daniel P . Berrangé]

On Wed, Jan 03, 2024 at 02:26:41PM +0100, Philippe Mathieu-Daudé wrote:
> On 3/1/24 13:26, Daniel P. Berrangé wrote:
> > Each VNC feature enum entry has a corresponding _MASK constant
> > which is the bit-shifted value. It is very easy for contributors
> > to accidentally use the _MASK constant, instead of the non-_MASK
> > constant, or the reverse. No compiler warning is possible and
> > it'll just silently do the wrong thing at runtime.
> > 
> > By introducing the vnc_set_feature helper method, we can drop
> > all the _MASK constants and thus prevent any future accidents.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   ui/vnc.c | 34 +++++++++++++++++-----------------
> >   ui/vnc.h | 21 ++++-----------------
> >   2 files changed, 21 insertions(+), 34 deletions(-)
> 
> 
> > @@ -599,6 +582,10 @@ static inline uint32_t vnc_has_feature(VncState *vs, 
> > int feature) {
> >       return (vs->features & (1 << feature));
> >   }
> > +static inline void vnc_set_feature(VncState *vs, int feature) {
> 
> Even stricter using s/int/VncFeatures/ enum type.

Even with that, the compiler happily lets you pass arbitrary int values
even if they're not mapped to VncFeature enum constants, so it doesn't
make it any stricter.  None the less it is beneficial as it makes it
self-documenting, so I'll change that.

> 
> With that:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> > +    vs->features |= (1 << feature);
> > +}
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|