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

[Christophe Fergeau]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151023/d3794c33/attachment.sig>