Re: [Qemu-devel] [PATCH v11 06/15] qapi: Visit variants in visit_type_FOO_fields()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/18/2016 06:58 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> We initially created the static visit_type_FOO_fields() helper
>>> function for reuse of code - we have cases where the initial
>>> setup for a visit has different allocation (depending on whether
>>> the fields represent a stand-alone type or are embedded as part
>>> of a larger type), but where the actual field visits are
>>> identical once a pointer is available.
>>>
>>> Up until the previous patch, visit_type_FOO_fields() was only
>>> used for structs (no variants), so it was covering every field
>>> for each type where it was emitted.
>>>
>>> Meanwhile, the code for visiting unions looks like:
>
> Maybe reword this to:
>
> Meanwhile, the previous patch updated the code for visiting unions to
> look like:

I don't mind.

>>> The resulting diff to the generated code is a bit hard to read,
>> 
>> Mostly because a few visit_type_T_fields() get generated in a different
>> place.  If I move them back manually for a diff, the patch's effect
>> becomes clear enough: the switch to visit the variants and the
>> visit_start_union() guarding it moves from visit_type_T() to
>> visit_type_T_fields().  That's what the commit message promises.
>
> I debated about splitting out a separate patch so that the motion of
> visit_type_T_fields() is separate from the variant visiting, but it
> wasn't worth the effort.  I'm glad you managed to see through the churn
> in the generated code.
>
>
>>> -def gen_visit_struct_fields(name, base, members):
>>> +def gen_visit_struct_fields(name, base, members, variants=None):
>> 
>> Two callers:
>> 
>> * gen_visit_union(): the new code here comes from there, except it's now
>>   completely guarded by if variants.  Effect: generated code motion.
>> 
>> * gen_visit_struct(): passes no variants, guard false, thus no change.
>> 
>> I think the patch becomes slightly clearer if you make the variants
>> parameter mandatory.  It'll probably become mandatory anyway when we
>> unify gen_visit_struct() and gen_visit_union().
>
> You beat me to it - yes, I realized that after sending the series.  We
> can either squash in the fix here to make variants mandatory (and
> gen_visit_struct() pass an explicit None, which disappears in the next
> patch), or squash in a fix to 7/15 to delete the '=None'.  Either way,
> I'm fine if you tackle that, if no other major findings turn up.

Can do.