Eric Blake <ebl...@redhat.com> writes: > On 07/28/2015 12:44 AM, Markus Armbruster wrote: > >>> >>>> +def gen_visit_union(name, base, variants): >>>> + ret = '' >>>> >>>> if base: >>>> - assert discriminator >>>> - base_fields = find_struct(base)['data'].copy() >>>> - del base_fields[discriminator] >>>> - ret += generate_visit_struct_fields(name, base_fields) >>>> + members = [m for m in base.members if m != variants.tag_member] >>>> + ret += generate_visit_struct_fields(name, members) >>> >>> Ouch. This hurts. If the same class is used as both the base class of a >>> flat union, and the base class of an ordinary struct, then the struct >>> tries to visit the base class, but no longer parses the field that the >>> union was using as its discriminator. >>> >>> We don't have any code that demonstrates this, but probably should. I >>> ran into it while working up my POC of what it would take to unbox >>> inherited structs (http://thread.gmane.org/gmane.comp.emulators.qemu/353204) >> >> Is this broken in master, or do my patches break it? >> >> Got a reproducer? > > Turns out I'm mistaken; we got lucky. The call to > generate_visit_struct_fields() creates a function for 'name' (aka the > union name), and not for 'base' (aka the class name that owns the > fields). So even if we have Base as a common struct between Child and > Union, the code emitted for Child generates visit_Base_fields(), while > the code emitted for Union generates visit_Union_fields(). > > So there is no reproducer, but _only_ as long as we reject unions as a > base class for any other object. And there is redundancy: we could > reuse visit_Base_fields() for the sake of the union, then avoid > (re-)visiting the discriminator, and we would no longer need to emit > visit_Union_fields(). But I can do that as part of the followup > cleanups; since I don't see anything broken with your patch, we don't > have to worry about it during this series.
Good. Thank you!
