[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discrimi
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator |
Date: |
Fri, 26 Jul 2013 09:13:01 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 |
On 07/26/2013 09:01 AM, Kevin Wolf wrote:
>>> if base:
>>> struct = find_struct(base)
>>> + if discriminator:
>>> + del struct['data'][discriminator]
>>
>> I asked before, but didn't get an answer; my question may just show my
>> unfamiliarity with python. Is this modifying the original 'struct',
>> such that other uses of the struct after this point will no longer
>> contain the discriminator key? Or is it only modifying a copy of
>> 'struct', with the original left intact? But based on the rest of your
>> patch...
>
> Sorry, this is in fact my own unfamiliarity with Python, combined with
> failure to fix all cases when you pointed it out. The only reason I
> didn't reply to that part of your review was that I thought it would be
> obvious when I send a fixed version. Well, except if I don't.
Ah, so this is a case of the blind leading the blind...
>
> I've changed this hunk now to match the other one:
>
> @@ -184,8 +186,13 @@ struct %(name)s
> ''')
>
> if base:
> - struct = find_struct(base)
> - ret += generate_struct_fields(struct['data'])
> + base_fields = find_struct(base)['data']
> + if discriminator:
> + base_fields = base_fields.copy()
> + del base_fields[discriminator]
> + ret += generate_struct_fields(base_fields)
> + else:
> + assert not discriminator
Yes, that makes more sense.
>> I think my findings are easy fixes; so I'm okay if you fix them and then
>> add:
>>
>> Reviewed-by: Eric Blake <address@hidden>
Which means this is indeed a valid review.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH 04/18] qapi-visit.py: Implement 'base' for unions, (continued)
- [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 06/18] qapi: Add visitor for implicit structs, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 05/18] docs: Document QAPI union types, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH v2 07/17] qapi: Flat unions with arbitrary discriminator, Kevin Wolf, 2013/07/26
- [Qemu-devel] [PATCH 09/18] qapi.py: Maintain a list of union types, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 08/18] qapi: Add consume argument to qmp_input_get_object(), Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 10/18] qapi: Anonymous unions, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 11/18] block: Allow "driver" option on the top level, Kevin Wolf, 2013/07/23