Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

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 <eblake@redhat.com>

Which means this is indeed a valid review.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]