[Spice-devel] [PATCH 09/14] server: make cursor channel private

[Frediano Ziglio]

> On Fri, 2015-10-23 at 10:29 -0400, Frediano Ziglio wrote:
> 
> > >  
> > > +#define RED_CHANNEL(Channel) ((RedChannel *)(Channel))
> > > +
> > 
> > I really don't like these kind of macros, they are really type
> > unsafe,
> > what about
> > 
> >   void *p = malloc(50);
> > 
> >   red_channel_client_disconnect(RED_CHANNEL(p));
> > 
> > the more explicit
> > 
> >   void *p = malloc(50);
> > 
> >   red_channel_client_disconnect((RedChannel*) p);
> > 
> > make it more explicit that we are doing something dangerous.
> > 
> > It would be much better to define a public structure like
> > 
> > struct CursorChannel {
> >    CommonChannel common;
> > };
> > 
> > and a private one (in cursor-channel.c) like
> > 
> > struct CursorChannel {
> >    CommonChannel common;
> >    ... any field needed ...
> > }
> > 
> > and define the macro as
> > 
> > #define RED_CHANNEL(Channel) (&(Channel)->common.base)
> > 
> 
> 
> But this just sets things up for the future conversion to GObject. When
> RedChannel is derived from GObject, RED_CHANNEL() will be the standard
> way to cast in a relatively typesafe manner.
> 
> Jonathon
> 

Fine, but is like posting two patches in a set where first patch introduce
a feature and the second fix it. Usually maintainer will ask to squash them.
The patches are quite far apart perhaps a comment would be enough.
It will help a future maintainer of the code which will look at the patch.
Note that I didn't propose to remove the patch but define in a more safe way.
I honestly prefer program code where a change cause a compile error to code
where small changes cause unexpected and hard to fix bugs.

Frediano