[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] qobject: modify qobject_ref() to assert on
Re: [Qemu-devel] [PATCH 2/2] qobject: modify qobject_ref() to assert on NULL
Fri, 24 Aug 2018 15:32:24 +0200
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.
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 ...
> 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).
[Qemu-devel] [PATCH 2/2] qobject: modify qobject_ref() to assert on NULL, Marc-André Lureau, 2018/08/17
- Re: [Qemu-devel] [PATCH 1/2] qdict: add qdict_steal(), (continued)