Re: [Qemu-devel] [PATCH v5 10/46] qapi: Merge generation of per-member visits

[Eric Blake]

On 09/28/2015 12:17 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Consolidate the code between visit, command marshalling, and
>> event generation that iterates over the members of a struct.
>> It reduces code duplication in the generator, with no change to
>> generated marshal code, slightly more verbose visit code:
>>
>> |     visit_optional(v, &(*obj)->has_device, "device", &err);
>> |-    if (!err && (*obj)->has_device) {
>> |-        visit_type_str(v, &(*obj)->device, "device", &err);
>> |-    }
>> |     if (err) {
>> |         goto out;
>> |     }
>> |+    if ((*obj)->has_device) {
>> |+        visit_type_str(v, &(*obj)->device, "device", &err);
>> |+        if (err) {
>> |+            goto out;
>> |+        }
>> |+    }
> 
> I think the more verbose code is easier to understand, because it checks
> for errors exactly the same way as we do all the time, mimimizing
> cognitive load.

And then I shorten it later in 27/46, but the shorter form is consistent
to all three due to this refactor into a common helper.

> 
>> and slightly more verbose event code (recall that the qmp
>> output visitor has a no-op visit_optional()):
>>
>> |+    visit_optional(v, &has_offset, "offset", &err);
>> |+    if (err) {
>> |+        goto out;
>> |+    }
> 
> If we had a written contract, I suspect not calling visit_optional()
> would be a bug.

Indeed - we got lucky that the output visitor's visit_optional() was a
no-op.  I'll make that fact more obvious in the commit message.


>>
>> -def gen_err_check(err):
>> -    if not err:
>> -        return ''
>> -    return mcgen('''
>> -if (%(err)s) {
>> -    goto out;
>> -}
>> -''',
>> -                 err=err)
>> -
>> -
> 
> Only code motion.

I'm actually debating about splitting the move of this helper function
into its own patch, and using it in more places. Part of my debate is
that I'd rather go with:

def gen_err_check(err='err', label='out'):
    if not err:
        return ''
    return mcgen('''
    if (%(err)s) {
        goto %(label)s;
    }
''',
                 err=err, label=label)

so that it is applicable in more places, and so that callers don't have
to worry about push_indent()/pop_indent() if it is at the default usage
of 4 spaces (right now, all callers have to push, and not just callers
at 8 spaces where it is embedded inside an 'if' block).

Hmm, and just writing that, I'm wondering if we should fix mcgen() to
eat leading whitespace on any final blank line [as a separate patch],
since at least for me, emacs wants me to indent as:

    return mcgen('''
    code
                 ''', args)

rather than with the closing ''' flush left.


>> -''')
>> +    ret += gen_visit_fields(arg_type.members, '', False, errarg)
> 
> Perhaps a bit neater: make parameters prefix='', need_cast=False, and
> say prefix=... and need_cast=True in the one call where you need it.

Good idea, will work it into my v6.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature