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 <[email protected]> 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
