[Spice-devel] [PATCH 5/9] server: dispatcher_init/dispatcher_new

[Frediano Ziglio]

> On Thu, Oct 22, 2015 at 05:48:58AM -0400, Frediano Ziglio wrote:
> > > 
> > > I am not sure if I understand your point here. Frediano.
> > > 
> > > For a cleaner code, red_dispatcher_new() must just create a dispatcher
> > > given the QXLInstance object, but I would prefer to set
> > > qxl->st->dispatcher out of this function.
> > > I mean, having something like: qxl->st->dispatcher =
> > > red_dispatcher_new(qxl);
> > > 
> > 
> > Yes, new patch add a line like this. Actually there is this line and also
> > qxl->st->dispatcher is set inside red_dispatcher_new.
> > I think that if the function is called red_dispatcher_new is a caller
> > responsibility
> > to check that qxl->st->dispatcher is NULL before calling it to avoid the
> > leak and
> > also to set qxl->st->dispatcher with the value returned by
> > red_dispatcher_new.
> > 
> > So in the end I would:
> > - remove qxl->st->dispatcher == NULL check inside red_dispatcher_new;
> 
> For what it's worth, if your API contract is "it's the caller
> responsibility to check that qxl->st->dispatcher is NULL before calling
> red_dispatcher_new", then g_return_val_if_fail(qxl->st->dispatcher ==
> NULL, NULL); means exactly that (ie it's a programming error to call
> this method if dispatcher is not NULL). The added benefit is that it's
> explicitly written, and if you in one year from now you rework that
> code/write new code forgetting this contract, then you'll get reminded
> with a loud warning.
> 
> Christophe
> 

No, if you put that line inside the function it became responsibility
of the function too.

I think all this confusion came from the fact that basically the
RedDispatcher is the internal implementation of QXLInstance.

Let make an example. Let say that you have a struct ArrayOfString and
a struct String. You have a string_new which build and initialize the
string structure. I bet you won't expect string_new checking if there
is a ArrayOfString with an empty slot for the new string. The array
is responsible to check this. In this case is easy to see the separation
and responsibility because you can see a string being used outside
the array. In our case the two objects are so bound that you are not
just creating a new object but extending and defining an existing one.
For this reason I don't see the *_new function as a good naming.

Frediano