Re: [Qemu-devel] [PATCH RFC v3 02/32] qapi: New QAPISchema intermediate reperesentation

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8332 bytes --]

On 08/04/2015 09:57 AM, Markus Armbruster wrote:
> The QAPI code generators work with a syntax tree (nested dictionaries)
> plus a few symbol tables (also dictionaries) on the side.
> 
> They have clearly outgrown these simple data structures.  There's lots
> of rummaging around in dictionaries, and information is recomputed on
> the fly.  For the work I'm going to do, I want more clearly defined
> and more convenient interfaces.
> 
> Going forward, I also want less coupling between the back-ends and the
> syntax tree, to make messing with the syntax easier.
> 
> 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.
> 
> Shortcut: the semantic analysis still relies on existing check_exprs()
> to do the actual semantic checking.  All this code needs to move into
> the classes.  Mark as TODO.
> 
> We generate array types eagerly, even though most of them aren't used.
> Mark as TODO.

I'm not sure if there are any array types that the rest of the code base
uses even though it doesn't appear as a directly used type within the
schemata.  Perhaps some of the builtin types (for example, if qom-get
needs to return ['uint8']). Besides builtins, maybe we can add some sort
of 'needs-array':'bool' key to each 'struct'/'union'/'enum'/'alternate',
which defaults to true only if the schema refers to the array type, but
which can be explicitly set to true to force the array type generation
even without a schema reference.  But as it is all properly marked TODO,
the idle ramblings in this paragraph don't affect review.

> 
> Nothing uses the new intermediate representation just yet, thus no
> change to generated files.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py       |   2 +-
>  scripts/qapi-event.py          |   2 +-
>  scripts/qapi-types.py          |   2 +-
>  scripts/qapi-visit.py          |   2 +-
>  scripts/qapi.py                | 361 ++++++++++++++++++++++++++++++++++++++++-
>  tests/qapi-schema/test-qapi.py |   2 +-
>  6 files changed, 357 insertions(+), 14 deletions(-)
> 

> +class QAPISchemaEnumType(QAPISchemaType):
> +    def __init__(self, name, info, values):
> +        QAPISchemaType.__init__(self, name, info)
> +        for v in values:
> +            assert isinstance(v, str)
> +        self.values = values
> +    def check(self, schema):
> +        assert len(set(self.values)) == len(self.values)

Doesn't check whether any of the distinct values map to the same C name.
 But not a show-stopper to this patch (the earlier semantic checking in
check_exprs() covers it, and your TODO about moving those checks here at
a later date is the right time to worry about it here).

> +
> +class QAPISchemaArrayType(QAPISchemaType):
> +    def __init__(self, name, info, element_type):
> +        QAPISchemaType.__init__(self, name, info)
> +        assert isinstance(element_type, str)
> +        self.element_type_name = element_type
> +        self.element_type = None
> +    def check(self, schema):
> +        self.element_type = schema.lookup_type(self.element_type_name)
> +        assert self.element_type

Is it worth adding:

assert not isinstance(self.element_type, QAPISchemaArrayType)

since we don't allow 2D arrays?

> +
> +class QAPISchemaObjectType(QAPISchemaType):
> +    def __init__(self, name, info, base, local_members, variants):
> +        QAPISchemaType.__init__(self, name, info)
> +        assert base == None or isinstance(base, str)
> +        for m in local_members:
> +            assert isinstance(m, QAPISchemaObjectTypeMember)
> +        if variants != None:
> +            assert isinstance(variants, QAPISchemaObjectTypeVariants)

Style nit: the 'base' and 'variants' checks are identical patterns
(checking for None or specific type), but only one uses an 'if'.
Possibly because of line-length issues, though, so I can live with it.

> +
> +class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> +    def __init__(self, name, typ):
> +        QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
> +    def check(self, schema, tag_type, seen):
> +        QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> +        assert self.name in tag_type.values
> +    # TODO try to get rid of .simple_union_type()
> +    def simple_union_type(self):
> +        if isinstance(self.type, QAPISchemaObjectType) and not self.type.info:
> +            assert len(self.type.members) == 1
> +            assert not self.type.variants # not implemented

and probably never will be (looks like this assert is copy-and-pasted
from other locations where it makes sense that we might implement
support for variants, but I don't see it ever happening for the
generated ':obj-*-wrapper' type for the branch of a simple union)

At any rate, I concur that we have a difference in the generated code
for simple unions compared to flat unions (even if simple unions could
be rewritten in terms of a flat union from the QMP side, the generated C
side is not the same), so I agree that you need this function for now,
as well as the comment for if we later try to clean up the code base to
drop that difference.

> +class QAPISchema(object):

> +    def _make_array_type(self, element_type):
> +        name = element_type + 'List'
> +        if not self.lookup_type(name):
> +            self._def_entity(QAPISchemaArrayType(name, None, element_type))
> +        return name

Hmm - we probably have collisions if a user tries to explicitly name a
'struct' or other type with a 'List' suffix.  Not made worse by this
patch and not an actual problem with any of our existing .json files, so
we can save it for another day.

> +
> +    def _make_members(self, data):
> +        return [self._make_member(key, data[key]) for key in data.keys()]

You could write this as:

return [self._make_member(key, value) for (key, value) in data.iteritems()]

for fewer copies and lookups (dict.keys() and dict.items() creates
copies, while dict.iteritems() visits key/value pairs in place).

I don't care strongly enough to hold up review, whether or not you
change it.

> +
> +    def _def_union_type(self, expr, info):
> +        name = expr['union']
> +        data = expr['data']
> +        base = expr.get('base')
> +        tag_name = expr.get('discriminator')
> +        tag_enum = None
> +        if tag_name:
> +            variants = [self._make_variant(key, data[key])
> +                        for key in data.keys()]
> +        else:
> +            variants = [self._make_simple_variant(key, data[key])
> +                        for key in data.keys()]

Same comments about the possibility for writing these list
comprehensions more efficiently.  [and "list comprehensions" is my
latest bit of cool python language terminology that I learned the name
of, in order to do this review]

> +    def _def_command(self, expr, info):
> +        name = expr['command']
> +        data = expr.get('data')
> +        rets = expr.get('returns')
> +        gen = expr.get('gen', True)
> +        success_response = expr.get('success-response', True)
> +        if isinstance(data, OrderedDict):
> +            data = self._make_implicit_object_type(name, 'arg',
> +                                                   self._make_members(data))
> +        if isinstance(rets, list):
> +            assert len(rets) == 1
> +            rets = self._make_array_type(rets[0])
> +        elif isinstance(rets, OrderedDict):
> +            rets = self._make_implicit_object_type(name, 'ret',
> +                                                   self._make_members(rets))

Dead 'elif' branch (since you reject dictionaries in "[PATCH 21/26]
qapi: Command returning anonymous type doesn't work, outlaw")

Looks like there will be some minor tweaks when you drop the RFC, but
it's close enough at this point that I'm okay with:

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]