| Message ID | 1453219845-30939-31-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Now that we elide unnecessary visits of empty types, we can > start using the special ':empty' type in more places. By using > the empty type as the base class of every explicit struct or > union, and as the default data for any command or event, we can > simplify later logic in qapi-{visit,commands,event} by merely > checking whether the type is empty, without also having to worry > whether a type was even supplied. > > Note that gen_object() in qapi-types still has to check for a > base, because it is also called for alternates (which have no > base). What about the one in gen_visit_struct()? if (base and not base.is_empty()) or members: ret += mcgen(''' visit_type_%(c_name)s_fields(v, obj, &err); ''', c_name=c_name(name)) > No change to generated code. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v9: squash in more related changes > v8: rebase to earlier changes > v7: rebase to earlier changes > v6: new patch > --- > scripts/qapi-commands.py | 17 +++++++------ > scripts/qapi-event.py | 5 ++-- > scripts/qapi-types.py | 4 +-- > scripts/qapi-visit.py | 12 +++++---- > scripts/qapi.py | 25 +++++++++--------- > tests/qapi-schema/event-case.out | 2 +- > tests/qapi-schema/flat-union-empty.out | 1 + > tests/qapi-schema/ident-with-escape.out | 1 + > tests/qapi-schema/indented-expr.out | 4 +-- > tests/qapi-schema/qapi-schema-test.out | 45 ++++++++++++++++++++++++++++++--- > tests/qapi-schema/union-clash-data.out | 2 ++ > tests/qapi-schema/union-empty.out | 1 + > 12 files changed, 83 insertions(+), 36 deletions(-) > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index 00ee565..9d455c3 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -29,11 +29,11 @@ def gen_call(name, arg_type, ret_type): > ret = '' > > argstr = '' > - if arg_type: > - for memb in arg_type.members: > - if memb.optional: > - argstr += 'has_%s, ' % c_name(memb.name) > - argstr += '%s, ' % c_name(memb.name) > + assert arg_type > + for memb in arg_type.members: The assertion doesn't buy us much. If arg_type is true, but of the wrong type, the assertion holds, and arg_type.members throws an error. If it false, then arg_type.members's error is just as useful as an AssertionError. More of the same below. > + if memb.optional: > + argstr += 'has_%s, ' % c_name(memb.name) > + argstr += '%s, ' % c_name(memb.name) > > lhs = '' > if ret_type: > @@ -65,7 +65,8 @@ def gen_marshal_vars(arg_type, ret_type): > ''', > c_type=ret_type.c_type()) > > - if arg_type and not arg_type.is_empty(): > + assert arg_type > + if not arg_type.is_empty(): > ret += mcgen(''' > QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); > QapiDeallocVisitor *qdv; > @@ -97,7 +98,7 @@ def gen_marshal_vars(arg_type, ret_type): > def gen_marshal_input_visit(arg_type, dealloc=False): > ret = '' > > - if not arg_type or arg_type.is_empty(): > + if arg_type.is_empty(): > return ret > > if dealloc: > @@ -177,7 +178,7 @@ def gen_marshal(name, arg_type, ret_type): > > # 'goto out' produced by gen_marshal_input_visit->gen_visit_fields() > # for each arg_type member, and by gen_call() for ret_type > - if (arg_type and not arg_type.is_empty()) or ret_type: > + if not arg_type.is_empty() or ret_type: > ret += mcgen(''' > > out: > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > index 56c93a8..cc55de7 100644 > --- a/scripts/qapi-event.py > +++ b/scripts/qapi-event.py > @@ -39,7 +39,8 @@ def gen_event_send(name, arg_type): > ''', > proto=gen_event_send_proto(name, arg_type)) > > - if arg_type and not arg_type.is_empty(): > + assert arg_type > + if not arg_type.is_empty(): > ret += mcgen(''' > QmpOutputVisitor *qov; > Visitor *v; > @@ -88,7 +89,7 @@ out_obj: > ''', > c_enum=c_enum_const(event_enum_name, name)) > > - if arg_type and not arg_type.is_empty(): > + if not arg_type.is_empty(): > ret += mcgen(''' > out: > qmp_output_visitor_cleanup(qov); > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index c70fae1..5cf20c2 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -58,7 +58,7 @@ struct %(c_name)s { > ''', > c_name=c_name(name)) > > - if base: > + if base and not base.is_empty(): > ret += mcgen(''' > /* Members inherited from %(c_name)s: */ > ''', Here, you go straight from X to X and not X.is_empty(). Elsewhere, you take two steps: one patch goes from X to X and X.is_empty(), changing ouput when X is an empty struct, and this one to just X.is_empty(), without changing output. I understand you need to keep checking base because alternates have none. I suspect that if base is empty, this hunk changes output, unlike the other ones, namely the comments /* Members inherited from %(c_name)s: */ /* Own members: */ go away. If that's true, it should either be a separate patch, or be mentioned in the commit message. A test case would be nice. > @@ -222,7 +222,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): > def visit_object_type(self, name, info, base, members, variants): > self._fwdecl += gen_fwd_object_or_array(name) > self.decl += gen_object(name, base, members, variants) > - if base: > + if not base.is_implicit(): Why .is_implicit() instead of .is_empty() here? > self.decl += gen_upcast(name, base) > self._gen_type_cleanup(name) > > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 6537a20..6d5c3d9 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -74,10 +74,11 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error * > def gen_visit_struct_fields(name, base, members): > ret = '' > > - if (not base or base.is_empty()) and not members: > + assert base > + if base.is_empty() and not members: > return ret > > - if base and not base.is_empty(): > + if not base.is_empty(): > ret += gen_visit_fields_decl(base) > > struct_fields_seen.add(name) > @@ -90,7 +91,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e > ''', > c_name=c_name(name)) > > - if base and not base.is_empty(): > + if not base.is_empty(): > ret += mcgen(''' > visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err); > ''', > @@ -246,7 +247,8 @@ out: > def gen_visit_union(name, base, variants): > ret = '' > > - if base: > + assert base > + if not base.is_empty(): > ret += gen_visit_fields_decl(base) Here, you go straight from X to X.is_empty(). Elsewhere, you take two steps: one patch goes from X to X and X.is_empty(), changing ouput when X is an empty struct, and this one to just X.is_empty(), without changing output. The first step is actually PATCH 29: it changes gen_visit_fields_decl() not to generate anything for an empty type. I think it should also make this call unconditional. > > for var in variants.variants: > @@ -270,7 +272,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error > ''', > c_name=c_name(name)) > > - if base: > + if not base.is_empty(): Another one, let's see what this does. > ret += mcgen(''' > visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err); > ''', c_name=base.c_name()) else: ret += mcgen(''' visit_type_%(c_type)s(v, "%(name)s", &(*obj)->%(c_name)s, &err); ''', c_type=variants.tag_member.type.c_name(), c_name=c_name(variants.tag_member.name), name=variants.tag_member.name) The condition is really flat union (the common members are in the base) vs. simple union (the common member is tag_member). If flat union, we generate the base visit, else the tag_member visit. Together, this generates the visit of common members. Your change breaks the condition: now simple unions have a base, too. You fix it up to test for empty base instead. Okay. > diff --git a/scripts/qapi.py b/scripts/qapi.py > index a13c110..e9006e4 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -922,11 +922,11 @@ class QAPISchemaArrayType(QAPISchemaType): > > class QAPISchemaObjectType(QAPISchemaType): > def __init__(self, name, info, base, local_members, variants): > - # struct has local_members, optional base, and no variants > + # struct has local_members, base, and no variants > # flat union has base, variants, and no local_members > - # simple union has local_members, variants, and no base > + # simple union has local_members, variants, and base of :empty > QAPISchemaType.__init__(self, name, info) > - assert base is None or isinstance(base, str) > + assert isinstance(base, str) or name == ':empty' No longer tight: if name == ':empty', base must be None, else it must be a str. > for m in local_members: > assert isinstance(m, QAPISchemaObjectTypeMember) > m.set_owner(name) > @@ -1264,11 +1264,11 @@ class QAPISchema(object): > > def _make_implicit_object_type(self, name, info, role, members): > if not members: > - return None > + return ':empty' > # See also QAPISchemaObjectTypeMember._pretty_owner() > name = ':obj-%s-%s' % (name, role) > if not self.lookup_entity(name, QAPISchemaObjectType): > - self._def_entity(QAPISchemaObjectType(name, info, None, > + self._def_entity(QAPISchemaObjectType(name, info, ':empty', > members, None)) > return name > > @@ -1295,7 +1295,7 @@ class QAPISchema(object): > > def _def_struct_type(self, expr, info): > name = expr['struct'] > - base = expr.get('base') > + base = expr.get('base', ':empty') > data = expr['data'] > self._def_entity(QAPISchemaObjectType(name, info, base, > self._make_members(data, info), > @@ -1315,7 +1315,7 @@ class QAPISchema(object): > def _def_union_type(self, expr, info): > name = expr['union'] > data = expr['data'] > - base = expr.get('base') > + base = expr.get('base', ':empty') > tag_name = expr.get('discriminator') > tag_member = None > if tag_name: > @@ -1349,11 +1349,11 @@ class QAPISchema(object): > > def _def_command(self, expr, info): > name = expr['command'] > - data = expr.get('data') > + data = expr.get('data', {}) expr.get('data', {}) returns a str or OrderedDict if expr has the key, else a dict. Unclean. > rets = expr.get('returns') > gen = expr.get('gen', True) > success_response = expr.get('success-response', True) > - if isinstance(data, OrderedDict): > + if isinstance(data, dict): The test works since OrderedDict is also a dict. > data = self._make_implicit_object_type( > name, info, 'arg', self._make_members(data, info)) I remember screwing up dict vs. OrderedDict once, and it failed when some code really expected an OrderedDict. _make_members() looks like it's okay with dict, but why risk it? Why not simply data = expr.get('data', ':empty') ? > if isinstance(rets, list): > @@ -1364,8 +1364,8 @@ class QAPISchema(object): > > def _def_event(self, expr, info): > name = expr['event'] > - data = expr.get('data') > - if isinstance(data, OrderedDict): > + data = expr.get('data', {}) > + if isinstance(data, dict): > data = self._make_implicit_object_type( > name, info, 'arg', self._make_members(data, info)) > self._def_entity(QAPISchemaEvent(name, info, data)) Same here. > @@ -1612,8 +1612,7 @@ extern const char *const %(c_name)s_lookup[]; > > > def gen_params(arg_type, extra): > - if not arg_type: > - return extra > + assert arg_type > assert not arg_type.variants > ret = '' > sep = '' [Test output diffs snipped...] Overall, a somewhat scary change, because all users of base need to be checked. The ones you actually changed are easy enough to see for a reviewer. Missed ones aren't. Fortunately, we got a decent test suite. Not sure this is worth the churn by itself, but let me read further to see it in more context.
Markus Armbruster <armbru@redhat.com> writes: > Eric Blake <eblake@redhat.com> writes: > >> Now that we elide unnecessary visits of empty types, we can >> start using the special ':empty' type in more places. By using >> the empty type as the base class of every explicit struct or >> union, and as the default data for any command or event, we can >> simplify later logic in qapi-{visit,commands,event} by merely >> checking whether the type is empty, without also having to worry >> whether a type was even supplied. >> >> Note that gen_object() in qapi-types still has to check for a >> base, because it is also called for alternates (which have no >> base). > > What about the one in gen_visit_struct()? > > if (base and not base.is_empty()) or members: > ret += mcgen(''' > visit_type_%(c_name)s_fields(v, obj, &err); > ''', > c_name=c_name(name)) > >> No change to generated code. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> v9: squash in more related changes >> v8: rebase to earlier changes >> v7: rebase to earlier changes >> v6: new patch >> --- >> scripts/qapi-commands.py | 17 +++++++------ >> scripts/qapi-event.py | 5 ++-- >> scripts/qapi-types.py | 4 +-- >> scripts/qapi-visit.py | 12 +++++---- >> scripts/qapi.py | 25 +++++++++--------- >> tests/qapi-schema/event-case.out | 2 +- >> tests/qapi-schema/flat-union-empty.out | 1 + >> tests/qapi-schema/ident-with-escape.out | 1 + >> tests/qapi-schema/indented-expr.out | 4 +-- >> tests/qapi-schema/qapi-schema-test.out | 45 ++++++++++++++++++++++++++++++--- >> tests/qapi-schema/union-clash-data.out | 2 ++ >> tests/qapi-schema/union-empty.out | 1 + >> 12 files changed, 83 insertions(+), 36 deletions(-) Missing: update to qapi-introspect.py. At least the expressions like arg_type or self._schema.the_empty_object_type need updating. I got stuck in reviewing PATCH 31. The result is probably fine, but the patch itself is impenetrable for me. I'm playing with the code to see whether I can turn that into a constructive remark.
Markus Armbruster <armbru@redhat.com> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> Eric Blake <eblake@redhat.com> writes: >> >>> Now that we elide unnecessary visits of empty types, we can >>> start using the special ':empty' type in more places. By using >>> the empty type as the base class of every explicit struct or >>> union, and as the default data for any command or event, we can >>> simplify later logic in qapi-{visit,commands,event} by merely >>> checking whether the type is empty, without also having to worry >>> whether a type was even supplied. You rewrite a command's arg_type from None to ':empty', bit not its ret_type. Deepens the assymmetry between the two. >>> Note that gen_object() in qapi-types still has to check for a >>> base, because it is also called for alternates (which have no >>> base). >> >> What about the one in gen_visit_struct()? >> >> if (base and not base.is_empty()) or members: >> ret += mcgen(''' >> visit_type_%(c_name)s_fields(v, obj, &err); >> ''', >> c_name=c_name(name)) >> >>> No change to generated code. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> >>> --- >>> v9: squash in more related changes >>> v8: rebase to earlier changes >>> v7: rebase to earlier changes >>> v6: new patch >>> --- >>> scripts/qapi-commands.py | 17 +++++++------ >>> scripts/qapi-event.py | 5 ++-- >>> scripts/qapi-types.py | 4 +-- >>> scripts/qapi-visit.py | 12 +++++---- >>> scripts/qapi.py | 25 +++++++++--------- >>> tests/qapi-schema/event-case.out | 2 +- >>> tests/qapi-schema/flat-union-empty.out | 1 + >>> tests/qapi-schema/ident-with-escape.out | 1 + >>> tests/qapi-schema/indented-expr.out | 4 +-- >>> tests/qapi-schema/qapi-schema-test.out | 45 ++++++++++++++++++++++++++++++--- >>> tests/qapi-schema/union-clash-data.out | 2 ++ >>> tests/qapi-schema/union-empty.out | 1 + >>> 12 files changed, 83 insertions(+), 36 deletions(-) > > Missing: update to qapi-introspect.py. At least the expressions like > > arg_type or self._schema.the_empty_object_type > > need updating. But so far not the ret_type or self._schema.the_empty_object_type. [...]
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 00ee565..9d455c3 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -29,11 +29,11 @@ def gen_call(name, arg_type, ret_type): ret = '' argstr = '' - if arg_type: - for memb in arg_type.members: - if memb.optional: - argstr += 'has_%s, ' % c_name(memb.name) - argstr += '%s, ' % c_name(memb.name) + assert arg_type + for memb in arg_type.members: + if memb.optional: + argstr += 'has_%s, ' % c_name(memb.name) + argstr += '%s, ' % c_name(memb.name) lhs = '' if ret_type: @@ -65,7 +65,8 @@ def gen_marshal_vars(arg_type, ret_type): ''', c_type=ret_type.c_type()) - if arg_type and not arg_type.is_empty(): + assert arg_type + if not arg_type.is_empty(): ret += mcgen(''' QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); QapiDeallocVisitor *qdv; @@ -97,7 +98,7 @@ def gen_marshal_vars(arg_type, ret_type): def gen_marshal_input_visit(arg_type, dealloc=False): ret = '' - if not arg_type or arg_type.is_empty(): + if arg_type.is_empty(): return ret if dealloc: @@ -177,7 +178,7 @@ def gen_marshal(name, arg_type, ret_type): # 'goto out' produced by gen_marshal_input_visit->gen_visit_fields() # for each arg_type member, and by gen_call() for ret_type - if (arg_type and not arg_type.is_empty()) or ret_type: + if not arg_type.is_empty() or ret_type: ret += mcgen(''' out: diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index 56c93a8..cc55de7 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -39,7 +39,8 @@ def gen_event_send(name, arg_type): ''', proto=gen_event_send_proto(name, arg_type)) - if arg_type and not arg_type.is_empty(): + assert arg_type + if not arg_type.is_empty(): ret += mcgen(''' QmpOutputVisitor *qov; Visitor *v; @@ -88,7 +89,7 @@ out_obj: ''', c_enum=c_enum_const(event_enum_name, name)) - if arg_type and not arg_type.is_empty(): + if not arg_type.is_empty(): ret += mcgen(''' out: qmp_output_visitor_cleanup(qov); diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index c70fae1..5cf20c2 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -58,7 +58,7 @@ struct %(c_name)s { ''', c_name=c_name(name)) - if base: + if base and not base.is_empty(): ret += mcgen(''' /* Members inherited from %(c_name)s: */ ''', @@ -222,7 +222,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): def visit_object_type(self, name, info, base, members, variants): self._fwdecl += gen_fwd_object_or_array(name) self.decl += gen_object(name, base, members, variants) - if base: + if not base.is_implicit(): self.decl += gen_upcast(name, base) self._gen_type_cleanup(name) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 6537a20..6d5c3d9 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -74,10 +74,11 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error * def gen_visit_struct_fields(name, base, members): ret = '' - if (not base or base.is_empty()) and not members: + assert base + if base.is_empty() and not members: return ret - if base and not base.is_empty(): + if not base.is_empty(): ret += gen_visit_fields_decl(base) struct_fields_seen.add(name) @@ -90,7 +91,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e ''', c_name=c_name(name)) - if base and not base.is_empty(): + if not base.is_empty(): ret += mcgen(''' visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err); ''', @@ -246,7 +247,8 @@ out: def gen_visit_union(name, base, variants): ret = '' - if base: + assert base + if not base.is_empty(): ret += gen_visit_fields_decl(base) for var in variants.variants: @@ -270,7 +272,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error ''', c_name=c_name(name)) - if base: + if not base.is_empty(): ret += mcgen(''' visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err); ''', diff --git a/scripts/qapi.py b/scripts/qapi.py index a13c110..e9006e4 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -922,11 +922,11 @@ class QAPISchemaArrayType(QAPISchemaType): class QAPISchemaObjectType(QAPISchemaType): def __init__(self, name, info, base, local_members, variants): - # struct has local_members, optional base, and no variants + # struct has local_members, base, and no variants # flat union has base, variants, and no local_members - # simple union has local_members, variants, and no base + # simple union has local_members, variants, and base of :empty QAPISchemaType.__init__(self, name, info) - assert base is None or isinstance(base, str) + assert isinstance(base, str) or name == ':empty' for m in local_members: assert isinstance(m, QAPISchemaObjectTypeMember) m.set_owner(name) @@ -1264,11 +1264,11 @@ class QAPISchema(object): def _make_implicit_object_type(self, name, info, role, members): if not members: - return None + return ':empty' # See also QAPISchemaObjectTypeMember._pretty_owner() name = ':obj-%s-%s' % (name, role) if not self.lookup_entity(name, QAPISchemaObjectType): - self._def_entity(QAPISchemaObjectType(name, info, None, + self._def_entity(QAPISchemaObjectType(name, info, ':empty', members, None)) return name @@ -1295,7 +1295,7 @@ class QAPISchema(object): def _def_struct_type(self, expr, info): name = expr['struct'] - base = expr.get('base') + base = expr.get('base', ':empty') data = expr['data'] self._def_entity(QAPISchemaObjectType(name, info, base, self._make_members(data, info), @@ -1315,7 +1315,7 @@ class QAPISchema(object): def _def_union_type(self, expr, info): name = expr['union'] data = expr['data'] - base = expr.get('base') + base = expr.get('base', ':empty') tag_name = expr.get('discriminator') tag_member = None if tag_name: @@ -1349,11 +1349,11 @@ class QAPISchema(object): def _def_command(self, expr, info): name = expr['command'] - data = expr.get('data') + data = expr.get('data', {}) rets = expr.get('returns') gen = expr.get('gen', True) success_response = expr.get('success-response', True) - if isinstance(data, OrderedDict): + if isinstance(data, dict): data = self._make_implicit_object_type( name, info, 'arg', self._make_members(data, info)) if isinstance(rets, list): @@ -1364,8 +1364,8 @@ class QAPISchema(object): def _def_event(self, expr, info): name = expr['event'] - data = expr.get('data') - if isinstance(data, OrderedDict): + data = expr.get('data', {}) + if isinstance(data, dict): data = self._make_implicit_object_type( name, info, 'arg', self._make_members(data, info)) self._def_entity(QAPISchemaEvent(name, info, data)) @@ -1612,8 +1612,7 @@ extern const char *const %(c_name)s_lookup[]; def gen_params(arg_type, extra): - if not arg_type: - return extra + assert arg_type assert not arg_type.variants ret = '' sep = '' diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out index 6350d64..18866d9 100644 --- a/tests/qapi-schema/event-case.out +++ b/tests/qapi-schema/event-case.out @@ -1,4 +1,4 @@ object :empty enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] prefix QTYPE -event oops None +event oops :empty diff --git a/tests/qapi-schema/flat-union-empty.out b/tests/qapi-schema/flat-union-empty.out index eade2d5..1a58e07 100644 --- a/tests/qapi-schema/flat-union-empty.out +++ b/tests/qapi-schema/flat-union-empty.out @@ -1,5 +1,6 @@ object :empty object Base + base :empty member type: Empty optional=False enum Empty [] enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out index 453e0b2..142cd19 100644 --- a/tests/qapi-schema/ident-with-escape.out +++ b/tests/qapi-schema/ident-with-escape.out @@ -1,5 +1,6 @@ object :empty object :obj-fooA-arg + base :empty member bar1: str optional=False enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] prefix QTYPE diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out index ce37ff5..5c25fcd 100644 --- a/tests/qapi-schema/indented-expr.out +++ b/tests/qapi-schema/indented-expr.out @@ -1,7 +1,7 @@ object :empty enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] prefix QTYPE -command eins None -> None +command eins :empty -> None gen=True success_response=True -command zwei None -> None +command zwei :empty -> None gen=True success_response=True diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index d8f9108..a2a3c8b 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -1,56 +1,78 @@ object :empty object :obj-EVENT_C-arg + base :empty member a: int optional=True member b: UserDefOne optional=True member c: str optional=False object :obj-EVENT_D-arg + base :empty member a: EventStructOne optional=False member b: str optional=False member c: str optional=True member enum3: EnumOne optional=True object :obj-__org.qemu_x-command-arg + base :empty member a: __org.qemu_x-EnumList optional=False member b: __org.qemu_x-StructList optional=False member c: __org.qemu_x-Union2 optional=False member d: __org.qemu_x-Alt optional=False object :obj-anyList-wrapper + base :empty member data: anyList optional=False object :obj-boolList-wrapper + base :empty member data: boolList optional=False object :obj-guest-get-time-arg + base :empty member a: int optional=False member b: int optional=True object :obj-guest-sync-arg + base :empty member arg: any optional=False object :obj-int16List-wrapper + base :empty member data: int16List optional=False object :obj-int32List-wrapper + base :empty member data: int32List optional=False object :obj-int64List-wrapper + base :empty member data: int64List optional=False object :obj-int8List-wrapper + base :empty member data: int8List optional=False object :obj-intList-wrapper + base :empty member data: intList optional=False object :obj-numberList-wrapper + base :empty member data: numberList optional=False object :obj-sizeList-wrapper + base :empty member data: sizeList optional=False object :obj-str-wrapper + base :empty member data: str optional=False object :obj-strList-wrapper + base :empty member data: strList optional=False object :obj-uint16List-wrapper + base :empty member data: uint16List optional=False object :obj-uint32List-wrapper + base :empty member data: uint32List optional=False object :obj-uint64List-wrapper + base :empty member data: uint64List optional=False object :obj-uint8List-wrapper + base :empty member data: uint8List optional=False object :obj-user_def_cmd1-arg + base :empty member ud1a: UserDefOne optional=False object :obj-user_def_cmd2-arg + base :empty member ud1a: UserDefOne optional=False member ud1b: UserDefOne optional=True alternate AltIntNum @@ -71,24 +93,28 @@ alternate AltStrInt alternate AltStrNum case s: str case n: number -event EVENT_A None -event EVENT_B None +event EVENT_A :empty +event EVENT_B :empty event EVENT_C :obj-EVENT_C-arg event EVENT_D :obj-EVENT_D-arg object Empty1 + base :empty object Empty2 base Empty1 enum EnumOne ['value1', 'value2', 'value3'] object EventStructOne + base :empty member struct1: UserDefOne optional=False member string: str optional=False member enum2: EnumOne optional=True object ForceArrays + base :empty member unused1: UserDefOneList optional=False member unused2: UserDefTwoList optional=False member unused3: TestStructList optional=False enum MyEnum [] object NestedEnumsOne + base :empty member enum1: EnumOne optional=False member enum2: EnumOne optional=True member enum3: EnumOne optional=False @@ -98,10 +124,12 @@ enum QEnumTwo ['value1', 'value2'] enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] prefix QTYPE object TestStruct + base :empty member integer: int optional=False member boolean: bool optional=False member string: str optional=False object UserDefA + base :empty member boolean: bool optional=False member a_b: int optional=True alternate UserDefAlternate @@ -109,9 +137,11 @@ alternate UserDefAlternate case s: str case i: int object UserDefB + base :empty member intb: int optional=False member a-b: bool optional=True object UserDefC + base :empty member string1: str optional=False member string2: str optional=False object UserDefFlatUnion @@ -127,6 +157,7 @@ object UserDefFlatUnion2 case value2: UserDefB case value3: UserDefA object UserDefNativeListUnion + base :empty member type: UserDefNativeListUnionKind optional=False case integer: :obj-intList-wrapper case s8: :obj-int8List-wrapper @@ -148,19 +179,23 @@ object UserDefOne member string: str optional=False member enum1: EnumOne optional=True object UserDefOptions + base :empty member i64: intList optional=True member u64: uint64List optional=True member u16: uint16List optional=True member i64x: int optional=True member u64x: uint64 optional=True object UserDefTwo + base :empty member string0: str optional=False member dict1: UserDefTwoDict optional=False object UserDefTwoDict + base :empty member string1: str optional=False member dict2: UserDefTwoDictDict optional=False member dict3: UserDefTwoDictDict optional=True object UserDefTwoDictDict + base :empty member userdef: UserDefOne optional=False member string: str optional=False object UserDefUnionBase @@ -168,12 +203,14 @@ object UserDefUnionBase member string: str optional=False member enum1: EnumOne optional=False object UserDefZero + base :empty member integer: int optional=False event __ORG.QEMU_X-EVENT __org.qemu_x-Struct alternate __org.qemu_x-Alt case __org.qemu_x-branch: str case b: __org.qemu_x-Base object __org.qemu_x-Base + base :empty member __org.qemu_x-member1: __org.qemu_x-Enum optional=False enum __org.qemu_x-Enum ['__org.qemu_x-value'] object __org.qemu_x-Struct @@ -181,8 +218,10 @@ object __org.qemu_x-Struct member __org.qemu_x-member2: str optional=False member wchar-t: int optional=True object __org.qemu_x-Struct2 + base :empty member array: __org.qemu_x-Union1List optional=False object __org.qemu_x-Union1 + base :empty member type: __org.qemu_x-Union1Kind optional=False case __org.qemu_x-branch: :obj-str-wrapper enum __org.qemu_x-Union1Kind ['__org.qemu_x-branch'] @@ -196,7 +235,7 @@ command guest-get-time :obj-guest-get-time-arg -> int gen=True success_response=True command guest-sync :obj-guest-sync-arg -> any gen=True success_response=True -command user_def_cmd None -> None +command user_def_cmd :empty -> None gen=True success_response=True command user_def_cmd0 Empty2 -> Empty2 gen=True success_response=True diff --git a/tests/qapi-schema/union-clash-data.out b/tests/qapi-schema/union-clash-data.out index f5752f4..19031a2 100644 --- a/tests/qapi-schema/union-clash-data.out +++ b/tests/qapi-schema/union-clash-data.out @@ -1,9 +1,11 @@ object :empty object :obj-int-wrapper + base :empty member data: int optional=False enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] prefix QTYPE object TestUnion + base :empty member type: TestUnionKind optional=False case data: :obj-int-wrapper enum TestUnionKind ['data'] diff --git a/tests/qapi-schema/union-empty.out b/tests/qapi-schema/union-empty.out index bdf17e5..57fcbd1 100644 --- a/tests/qapi-schema/union-empty.out +++ b/tests/qapi-schema/union-empty.out @@ -2,5 +2,6 @@ object :empty enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] prefix QTYPE object Union + base :empty member type: UnionKind optional=False enum UnionKind []
Now that we elide unnecessary visits of empty types, we can start using the special ':empty' type in more places. By using the empty type as the base class of every explicit struct or union, and as the default data for any command or event, we can simplify later logic in qapi-{visit,commands,event} by merely checking whether the type is empty, without also having to worry whether a type was even supplied. Note that gen_object() in qapi-types still has to check for a base, because it is also called for alternates (which have no base). No change to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> --- v9: squash in more related changes v8: rebase to earlier changes v7: rebase to earlier changes v6: new patch --- scripts/qapi-commands.py | 17 +++++++------ scripts/qapi-event.py | 5 ++-- scripts/qapi-types.py | 4 +-- scripts/qapi-visit.py | 12 +++++---- scripts/qapi.py | 25 +++++++++--------- tests/qapi-schema/event-case.out | 2 +- tests/qapi-schema/flat-union-empty.out | 1 + tests/qapi-schema/ident-with-escape.out | 1 + tests/qapi-schema/indented-expr.out | 4 +-- tests/qapi-schema/qapi-schema-test.out | 45 ++++++++++++++++++++++++++++++--- tests/qapi-schema/union-clash-data.out | 2 ++ tests/qapi-schema/union-empty.out | 1 + 12 files changed, 83 insertions(+), 36 deletions(-)