[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 10/46] qapi: Merge generation of per-member v
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v5 10/46] qapi: Merge generation of per-member visits |
Date: |
Mon, 28 Sep 2015 09:40:24 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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
- Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call, (continued)
[Qemu-devel] [PATCH v5 08/46] qapi: Reuse code for flat union base validation, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 09/46] qapi: Use consistent generated code patterns, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 10/46] qapi: Merge generation of per-member visits, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 11/46] qapi: Don't use info as witness of implicit object type, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 12/46] qapi: Track location that created an implicit type, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 14/46] qapi: Detect collisions in C member names, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 16/46] qapi: Detect base class loops, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 15/46] qapi: Defer duplicate member checks to schema check(), Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 13/46] qapi: Track owner of each object member, Eric Blake, 2015/09/21