[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check()
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check() |
Date: |
Tue, 29 Sep 2015 09:15:53 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
On 09/29/2015 08:31 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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 <address@hidden>
>
> 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
signature.asc
Description: OpenPGP digital signature