Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] qobject: modify qobject_ref() to assert on NULL

[Marc-André Lureau]

Hi
On Fri, Aug 24, 2018 at 3:24 PM Markus Armbruster <address@hidden> wrote:
>
> Marc-André Lureau <address@hidden> writes:
>
> > Hi
> > On Fri, Aug 24, 2018 at 10:12 AM Markus Armbruster <address@hidden> wrote:
> >>
> >> Marc-André Lureau <address@hidden> writes:
> >>
> >> > While it may be convenient to accept NULL value in
> >> > qobject_unref() (for similar reasons as free() accepts NULL), it is
> >> > not such a good idea for qobject_ref(): now assert() on NULL.
> >>
> >> Why is it not such a good idea?
> >>
> >> Is it unsafe?  Unclean?  Ugly?
> >>
> >> If unsafe, can you point to actual problems, present or past?
> >>
> >> If you consider it unclean or ugly, can you point to established
> >> convention?
> >
> > In general, a function/method shouldn't accept NULL as argument, as it
> > is usually a programmer mistake.
>
> There are certainly plenty of specific functions where the claim is
> true.  But a blanket claim like yours need to be supported by evidence.

There is no evidence for such claim, only idiomatic code or common
patterns. It's clear when the caller handles/expects NULL cases.
That's what the patch does, there are a few places where this is made
explicit, and imho that improves readability.

For ex:
            evstate->data = qobject_ref(data);

You might assume, by just reading that line, that evstate->data != NULL.

But if you rewrite:

            evstate->data = data ? qobject_ref(data) : NULL;

Then it's clear that field accpets both cases, and programmer has
thought about that case, it's not handled by mistake (where it could
be a bug).


>
> > free() / unref() are exceptions, for convenience.
>
> Here's the argument for cleanup functions accepting null pointers.
>
> Having cleanup functions reject null pointers complicates cleanup for no
> gain.  Examples of this are everywhere.  Common pattern:
>
>     T1 *v1 = NULL;
>     ...
>     Tn *vn = NULL;
>
>     ... do stuff, create v1, ..., vn, goto out on error ...
>
> out:
>     T1_cleanup(v1);
>     ...
>     Tn_cleanup(vn);
>
> Guarding the Ti_cleanup() with if (vi) clutters the code.  Moving the
> guard into the Ti_cleanup() is an obvious improvement.
>
> Conversely, examples where a cleanup function rejecting null would catch
> actual mistakes do not come to mind.
>
> qobject_unref() is an instance of this pattern.
>
> qobject_ref() isn't, but there are similarities.  Again, examples exist
> where rejecting null buys us nothing but additional code to guard the
> call.  These are visible in your patch.  And again, examples where
> rejecting null would catch mistakes do not come to (my) mind.  That's
> why I'm asking for them.
>
> > fwiw, g_object_ref/unref() do not accept NULL. However,
> > g_clear_object() was introduced later to simply clearing an object
> > reference (unref and set to NULL, checks NULL).
>
> [...]



-- 
Marc-André Lureau