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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

Could have a parameter indent_amount with a suitable default, passed on
to push_indent() / pop_indent().

Our helper function's indentation level is rather haphazard.

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

I think Emacs is mistaken here.  I find the indented ''' mildly
distracting, because it immediately begs the question whether the
trailing whitespace ends up in the generated code.

If we want to add further processing, such as stripping trailing
whitespace, the correct place is cgen().  If we decide to strip it, I
guess we better strip it from every line, not just the last one.

However, I think the appropriate place to strip whitespace is the source
code.

See also commit 77e703b.

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