On 07/27/2015 03:23 AM, Markus Armbruster wrote:

>>>
>>> Create a bunch of classes to represent QAPI schemata.
>>>
>>> Have the QAPISchema initializer call the parser, then walk the syntax
>>> tree to create the new internal representation, and finally perform
>>> semantic analysis.
>>>

>>> +class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>>> +    def __init__(self, name, typ, flat):
>>> +        QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
>>> +        assert isinstance(flat, bool)
>>> +        self.flat = flat
>>> +    def check(self, schema, tag_type, seen):
>>> +        QAPISchemaObjectTypeMember.check(self, schema, [], seen)
>>> +        assert self.name in tag_type.values
>>> +        if self.flat:
>>> +            self.type.check(schema)
>>> +            assert isinstance(self.type, QAPISchemaObjectType)
>>
>> Do we really want to be tracking self.flat for each variant?  After all,
>> docs/qapi-code-gen.txt already describes the mapping from simple union
>> into flat union (as if the flat union had a base class with single
>> member 'kind' of the right type, then each branch of the union composed
>> of an implicit object with a lone member 'data' of the correct type).
>> In other words, is it any better to just normalize into that form now,
>> such that each QAPISchemaObjectTypeVariant is merely a (often
>> one-element) list of name:type members being added to the overall
>> QAPISchemaObject?
> 
> I tried to do exactly that, but got bogged down in special cases and
> copped out.  Then I went on vacation, and now I don't remember the exact
> problems anymore %-}
> 
> I guess / hope it's just relatively pointless differences in the
> generated C code I didn't want to get rid of at this time.  The series
> is long and hairy enough as it is...
> 
>>                    But I guess it remains to be seen how you use
>> self.flat before knowing if it is worth normalizing away from it.
> 
> At least introspect.json is oblivious of it.

Yeah, that was my conclusion by the end of the series - we still had
enough special cases where we generate different code for simple unions
than for flat unions. It would be possible to merge the generator and
simplify things further, but at this point, it is best done as a
follow-up series because it will touch lots of C code (anything that
uses a simple union would have to convert to the common representation).

And you actually did reference .flat in the patch that first added
qapi-introspect.py (good that it did not leak to the introspection
output, but it did have to be considered when figuring out what to
output); again, something you may want to rework before polishing this
into v3, or something you may end up just documenting as a possible
cleanup for a future series. But after having reviewed the whole series
now, I'm able to live with your use of .flat, if only because it makes
the initial conversion faster.

>>> +
>>> +    def lookup_entity(self, name, typ=None):
>>> +        ent = self.entity_dict.get(name)
>>> +        if typ and not isinstance(ent, typ):
>>> +            return None
>>> +        return ent
>>
>> Ah, so you enforce a shared namespace between types, commands, and
>> events (all three are in self.entity_dict), but can use the typ
>> parameter to allow limiting a lookup to just types.
> 
> Yes.  It's a convenience feature.

Documentation comments never hurt :) (You did mention in the cover
letter that part of the reason this was still RFC was that it was
lacking documentation)

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