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

[Markus Armbruster]

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.

> 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).

[...]