Re: [Qemu-devel] [PATCH v8 05/17] qapi: Track simple union tag in object.local_members

[Eric Blake]

On 10/30/2015 06:54 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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 <address@hidden>
>>

>> +++ 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