Eric Blake <ebl...@redhat.com> writes:
> 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.
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 <ebl...@redhat.com>
>>
>> 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.
