Re: [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2936 bytes --]

On 09/29/2015 08:31 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> qapi-commands had a nice helper gen_err_check(), but did not
> 
> In fact, it still has :)
> 
>> use it everywhere. In fact, using it in more places makes it
>> easier to reduce the lines of code used in appending an error
> 
> Suggest "used for generating error checks"
> 
>> check in generated code (previously required a multi-line
>> mcgen(), which didn't add any use of parameterization), which
>> in turn makes it easier to write the next patch which will
>> consolidate another common pattern among the generators.
> 
> I think we should burn some of the whiches ;)

Yay - a which-hunt.  I'll get the pitchfork.

> 
> Drop the paranthesis?
> 
>> The diffstat of this patch doesn't quite show as big a
>> reduction in lines as I had hoped, but that is in part due to
> 
> Almost none...
> 
>> the duplication of some FIXME comments.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Have you diffed the generated code before and after the patch?

Oops, forgot to mention. No change to generated code.


>> @@ -170,9 +159,10 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
>>
>>      v = qmp_output_get_visitor(qov);
>>      visit_type_%(c_name)s(v, &ret_in, "unused", &err);
>> -    if (err) {
>> -        goto out;
>> -    }
>> +''',
>> +                c_type=ret_type.c_type(), c_name=ret_type.c_name())
>> +    ret += gen_err_check()
>> +    ret += mcgen('''
>>      *ret_out = qmp_output_get_qobject(qov);
>>
>>   out:
> 
> Here's a case that becomes more verbose, so it's not just comment
> duplication.  Also becomes a bit harder to read, I think.

Due to splitting a larger chunk into smaller pieces to inject the error
in the middle.

I guess we could pick and choose to use the function only where it
doesn't split an already-existing large block, in order to maximize the
diffstat ratio rather than in favor of eliminating the common code
pattern duplication?

> 
> I don't know.  Could be worthwhile if it really makes further work
> easier.
> 
> To really cut the verbosity, I figure we'd have to do something more
> radical, like having cgen() recognize a (short!) pattern and replace it
> with a full-blown error check.  Not sure that's actually a good idea,
> though :)

In v5, it was only used by the shared gen_visit_fields(), until I added
uses of it later in the series to make anonymous base classes of a flat
union easier to implement.

I guess the conservative thing is to scale this patch back a bit (move
the function to the common location, and use it where it makes an
obvious difference to the diffstat, but leaving other places alone for
now).  Should I try that for v7?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]