Re: [Qemu-devel] [PATCH RFC v2 21/47] qapi: New QAPISchema intermediate reperesentation
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 signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH RFC v2 21/47] qapi: New QAPISchema intermediate reperesentation
Eric Blake ebl...@redhat.com writes: On 07/01/2015 02:22 PM, Markus Armbruster wrote: The QAPI code generators work with a syntax tree (nested dictionaries) plus a few symbol tables (also dictionaties) on the side. s/dictionaties/dictionaries/ Will fix. 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. Indeed - should be a lot easier to add new qapi .json syntax sugar on the front-end, or to tweak generated C layout on the backend, without close coupling between the two. Particularly when adding front-end sugar extensions that still normalize into the same backend structs. 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. No change to generated files at this stage in the game (this is mostly additive, then later patches shed their old ways of doing things by using this). Good division of labor between patches. I'll steal from this paragraph for my commit message. 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| 351 +++-- tests/qapi-schema/test-qapi.py | 2 +- 6 files changed, 347 insertions(+), 14 deletions(-) +++ b/scripts/qapi.py + +# +# Schema compiler frontend +# + +class QAPISchemaEntity(object): +def __init__(self, name, info): +assert isinstance(name, str) +self.name = name +self.info = info +def check(self, schema): +pass + +class QAPISchemaType(QAPISchemaEntity): +pass + +class QAPISchemaBuiltinType(QAPISchemaType): +def __init__(self, name): +QAPISchemaType.__init__(self, name, None) + +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 worth a check() method to ensure values don't collide? Especially since you already do that in QAPISchemaObjectTypeMember.check() Can do. Aside: everything the check() methods assert must be established by semantic analysis. Moving semantic analysis into the classes as outlined in the commit message will hopefully make that obvious enough to permit dropping the assertions. + +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 + +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) +self.base_name = base +self.base = None +self.local_members = local_members +self.variants = variants +self.members = None +def check(self, schema): +if self.members: +return # already checked +assert self.members == None # not running in cycles +self.members = False# mark as being checked Interesting, but makes sense (since we allow self-referential nesting, populating our own members may require revisiting the same type). Yes. Another way to put it: for a recursive type like this, we better check the recursion terminates. Cute that python allows both None and False as distinct non-true settings, so that you end up using the variable as a tri-state. Yup :) +if self.base_name: +self.base = schema.lookup_type(self.base_name) +
Re: [Qemu-devel] [PATCH RFC v2 21/47] qapi: New QAPISchema intermediate reperesentation
On 07/01/2015 02:22 PM, Markus Armbruster wrote: The QAPI code generators work with a syntax tree (nested dictionaries) plus a few symbol tables (also dictionaties) on the side. s/dictionaties/dictionaries/ 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. Indeed - should be a lot easier to add new qapi .json syntax sugar on the front-end, or to tweak generated C layout on the backend, without close coupling between the two. Particularly when adding front-end sugar extensions that still normalize into the same backend structs. 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. No change to generated files at this stage in the game (this is mostly additive, then later patches shed their old ways of doing things by using this). Good division of labor between patches. 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| 351 +++-- tests/qapi-schema/test-qapi.py | 2 +- 6 files changed, 347 insertions(+), 14 deletions(-) +++ b/scripts/qapi.py + +# +# Schema compiler frontend +# + +class QAPISchemaEntity(object): +def __init__(self, name, info): +assert isinstance(name, str) +self.name = name +self.info = info +def check(self, schema): +pass + +class QAPISchemaType(QAPISchemaEntity): +pass + +class QAPISchemaBuiltinType(QAPISchemaType): +def __init__(self, name): +QAPISchemaType.__init__(self, name, None) + +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 worth a check() method to ensure values don't collide? Especially since you already do that in QAPISchemaObjectTypeMember.check() + +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 + +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) +self.base_name = base +self.base = None +self.local_members = local_members +self.variants = variants +self.members = None +def check(self, schema): +if self.members: +return # already checked +assert self.members == None # not running in cycles +self.members = False# mark as being checked Interesting, but makes sense (since we allow self-referential nesting, populating our own members may require revisiting the same type). Cute that python allows both None and False as distinct non-true settings, so that you end up using the variable as a tri-state. +if self.base_name: +self.base = schema.lookup_type(self.base_name) +self.base.check(schema) Do you need 'assert self.base' here, similar to how you did it in QAPISchemaArrayType.check()? I guess you'll still get a python exception that None.check() doesn't exist if the lookup_type failed, so it just depends on what error message you want. +members = list(self.base.members) For that matter, do you want to assert isinstance(self.base, QAPISchemaObjectType), since lookup_type can return other types, but other child classes of QAPISchemaType do not have a .members? +else: +members = [] +
[Qemu-devel] [PATCH RFC v2 21/47] qapi: New QAPISchema intermediate reperesentation
The QAPI code generators work with a syntax tree (nested dictionaries)
plus a few symbol tables (also dictionaties) 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.
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| 351 +++--
tests/qapi-schema/test-qapi.py | 2 +-
6 files changed, 347 insertions(+), 14 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 223216d..3cdbd97 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -276,7 +276,7 @@ for o, a in opts:
if o in (-m, --middle):
middle_mode = True
-exprs = parse_schema(input_file)
+exprs = QAPISchema(input_file).get_exprs()
commands = filter(lambda expr: expr.has_key('command'), exprs)
commands = filter(lambda expr: not expr.has_key('gen'), commands)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 7f238df..aec2d32 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -263,7 +263,7 @@ fdecl.write(mcgen('''
''',
prefix=prefix))
-exprs = parse_schema(input_file)
+exprs = QAPISchema(input_file).get_exprs()
event_enum_name = c_name(prefix + QAPIEvent, protect=False)
event_enum_values = []
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4cc17da..a48ad9c 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -325,7 +325,7 @@ fdecl.write(mcgen('''
#include stdint.h
'''))
-exprs = parse_schema(input_file)
+exprs = QAPISchema(input_file).get_exprs()
fdecl.write(guardstart(QAPI_TYPES_BUILTIN_STRUCT_DECL))
for typename in builtin_types.keys():
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 427819a..a52a572 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -442,7 +442,7 @@ fdecl.write(mcgen('''
''',
prefix=prefix))
-exprs = parse_schema(input_file)
+exprs = QAPISchema(input_file).get_exprs()
# to avoid header dependency hell, we always generate declarations
# for built-in types in our header files and simply guard them
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 390ccd0..a80892e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -302,6 +302,7 @@ class QAPISchemaParser(object):
#
# Semantic analysis of schema expressions
+# TODO fold into QAPISchema
#
def find_base_fields(base):
@@ -751,15 +752,347 @@ def check_exprs(exprs):
else:
assert False, 'unexpected meta type'
-return map(lambda expr_elem: expr_elem['expr'], exprs)
-
-def parse_schema(fname):
-try:
-schema = QAPISchemaParser(open(fname, r))
-return check_exprs(schema.exprs)
-except (QAPISchemaError, QAPIExprError), e:
-print sys.stderr, e
-exit(1)
+return exprs
+
+#
+# Schema compiler frontend
+#
+
+class QAPISchemaEntity(object):
+def __init__(self, name, info):
+assert isinstance(name, str)
+self.name = name
+self.info = info
+def check(self, schema):
+pass
+
+class QAPISchemaType(QAPISchemaEntity):
+pass
+
+class QAPISchemaBuiltinType(QAPISchemaType):
+def __init__(self, name):
+QAPISchemaType.__init__(self, name, None)
+
+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
+
+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
+
+class QAPISchemaObjectType(QAPISchemaType):
+def __init__(self, name, info, base, local_members, variants):
+QAPISchemaType.__init__(self, name, info)
+
