Eric Blake <ebl...@redhat.com> writes:
> 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.
Because code for built-ins can be shared among schemata (see -b), we
probably have to keep generating code for array of built-in type
unconditionally.
Re needs-array: stupidest solution that could possibly work is to have
an otherwise useless struct mention all the needed arrays.
>> Nothing uses the new intermediate representation just yet, thus no
>> change to generated files.
>>
>> Signed-off-by: Markus Armbruster <arm...@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.
Pervasive issue.
> 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?
If the generators actually rely on it, yes.
If it's just an arbitrary schema language restriction, probably no.
>> +
>> +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.
I'll clean it up.
>> +
>> +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)
True. I'll drop the comment.
> 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.
That's the plan.
>> +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.
qapi-code-gen.txt reserves the 'Kind' suffix.
We should either adopt a sane, non-colliding scheme for generated names,
or prevent collisions by rejecting reserved names with a sane error
message (documenting them is then optional), or document reserved names.
The latter two require us to figure out what names we reserve. But as
you say, it's a task 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.
I'll check it out.
>> +
>> + 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.
There are a few more. I'll either change all or none.
> [and "list comprehensions" is my
> latest bit of cool python language terminology that I learned the name
> of, in order to do this review]
If I remember correctly, I first met them in an obscure hybrid of Prolog
and ML that was at the time a Professor's pet project, which naturally
meant every student must experience it, implemented in Lisp by a gifted
grad student, on an underpowered, memory-starved Mac. Let's say not
every student was able to see the beauty within the beast.
>> + 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")
Yup, will drop it.
> 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 <ebl...@redhat.com>
Thanks!
