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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

I think you'll need a pichfork for this ;-)

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

Let's state it in the commit message.

>>> @@ -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?

Two good reasons for capturing common code in a helper: one, ensuring
consistency and making it easier to change, and two, readability.

I don't think our "if (err) goto out" idiom is likely to change.  That
leaves just readability, I think.  A case-by-case approach makes sense
then.

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

Yes, please.  Except let yourself be guided by your eye, not by
diffstat.