[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v11 06/15] qapi: Visit variants in visit_type_FO
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v11 06/15] qapi: Visit variants in visit_type_FOO_fields() |
Date: |
Thu, 18 Feb 2016 18:04:14 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.
[Qemu-devel] [PATCH v11 10/15] qapi: Emit structs used as variants in topological order, Eric Blake, 2016/02/18
[Qemu-devel] [PATCH v11 09/15] qapi: Adjust layout of FooList types, Eric Blake, 2016/02/18
[Qemu-devel] [PATCH v11 14/15] qapi: Delete visit_start_union(), gen_visit_implicit_struct(), Eric Blake, 2016/02/18
[Qemu-devel] [PATCH v11 08/15] qapi-visit: Less indirection in visit_type_Foo_fields(), Eric Blake, 2016/02/18
[Qemu-devel] [PATCH v11 11/15] qapi-visit: Use common idiom in gen_visit_fields_decl(), Eric Blake, 2016/02/18