Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/06/2014 07:30 AM, Markus Armbruster wrote:
>> Visitors get passed a pointer to the visited object.  The generated
>> visitors try to cope with this pointer being null in some places, for
>> instance like this:
>> 
>>     visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
>> 
>> visit_start_optional() passes its second argument to Visitor method
>> start_optional.  Two out of two methods dereference it
>> unconditionally.
>
> Let's see if I can find them without Coverity's help...
>
> opts-visitor.c:opts_start_optional
> qmp-input-visitor.c:qmp_input_start_optional
> string-input-visitor.c:parse_start_optional
>
> Your counting is off :)  All three unconditionally dereference.

Right.  I can't count :)

Most Coverity's defects are about silliness like this:

    visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
    if (obj && (*obj)->has_name) {
        visit_type_str(m, obj ? &(*obj)->name : NULL, "name", &err);
    }

The second "obj ? ... : ..." is guarded by if (obj), thus cannot take
the false branch.

> [While at it, this code style from parse_start_optional, and similar in
> other locations, is rather verbose:
>
>     if (!siv->string) {
>         *present = false;
>         return;
>     }
>
>     *present = true;
> }
>
> Shorter would be:
>
>     *present = !!siv->string;
> }
>
> but that doesn't affect this patch]

Coccinelle job, if I can teach it our macros.  Without that, it
frequently chokes on some macro usage, leaving too much code unexamined.

>> I fail to see how hits pointer could legitimately be null.
>> 
>> All this useless null checking is highly redundant, which Coverity
>> duly reports.  About 200 times.
>> 
>> Remove the useless null checks.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  scripts/qapi-visit.py | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> Reviewed-by: Eric Blake <address@hidden>