On 10/30/2015 06:54 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> We were previously creating all unions with an empty list for >> local_members. However, it will make it easier to unify struct >> and union generation if we include the generated tag member in >> local_members. That way, we can have a common code pattern: >> visit the base (if any), visit the local members (if any), visit >> the variants (if any). The local_members of a flat union >> remains empty (because the discriminator is already visited as >> part of the base). > > Keeping the implicit tag implicit by not including it in local_members > was a conscious design decision, but if including it makes unifying > struct and union into objects easier, go right ahead. > >> The various front end entities then map as follows: >> struct: optional base, optional local_members, no variants >> simple union: no base, one-element local_members, variants with tag_member >> from local_members >> flat union: base, no local_members, variants with tag_member from base >> alternate: no base, no local_members, variants >> >> With the new local members, we require a bit of finesse to >> avoid assertions in the clients. No change to generated code. >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >>
>> +++ b/scripts/qapi-types.py >> @@ -269,7 +269,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): >> def visit_object_type(self, name, info, base, members, variants): >> self._fwdecl += gen_fwd_object_or_array(name) >> if variants: >> - assert not members # not implemented >> + if members: >> + assert len(members) == 1 >> + assert members[0] == variants.tag_member >> self.decl += gen_union(name, base, variants) >> else: >> self.decl += gen_struct(name, base, members) > > The assertion checks that not passing members to gen_union() won't lose > any. Before the patch, we assert there are none. After the patch, we > assert there's either none or variants.tag_member. Before ans after, > gen_union() takes care of variants.tag_member. Okay. > > Let's keep the comment, though. Perhaps like this: > > # Members other than variants.tag_member not implemented > assert len(members) == 1 > assert members[0] == variants.tag_member Indeed, that sounds nicer. > > I hope this the whole conditional will eventually be replaced by > > self.decl += gen_object(name, base, members, variants) Yes. It goes away in 6/17 for qapi-types (as you already saw), and I have a later patch queued that likewise makes it go away for qapi-visit (but not in subset C or D, since there's still a bit more to clean up there first). >> def check(self, schema, members, seen): >> - if self.tag_name: >> + if self.tag_name: # flat union >> self.tag_member = seen[self.tag_name] >> - else: >> + elif seen: # simple union >> + assert self.tag_member in seen.itervalues() >> + else: # alternate >> self.tag_member.check(schema, members, seen) >> assert isinstance(self.tag_member.type, QAPISchemaEnumType) >> for v in self.variants: > > The test for simple union is hackish. > > I guess you want to bypass self.tag_member.check() when for simple > unions, because it's now checked when QAPISchemaObjectType.check() calls > QAPISchemaObjectTypeMember.check() for each of self.local_members. > > Could we instead make QAPISchemaAlternateType.check() call > QAPISchemaObjectTypeMember.check() for its implicit tag, and drop the > else here? I debated about that when writing my patch. Yes, I can make that change (it seems a little bit unclean to be calling self.variants.tag_member.check() in QAPISchemaAlternateType, but not too much worse; and adding a comment might help). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
