On 09/29/2015 08:31 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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 <ebl...@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
signature.asc
Description: OpenPGP digital signature
