From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42763) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNmRA-0004HI-PG for qemu-devel@nongnu.org; Mon, 25 Jan 2016 14:04:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aNmR6-0001YO-Hq for qemu-devel@nongnu.org; Mon, 25 Jan 2016 14:04:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47855) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNmR6-0001YG-7V for qemu-devel@nongnu.org; Mon, 25 Jan 2016 14:04:24 -0500 From: Markus Armbruster References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-31-git-send-email-eblake@redhat.com> Date: Mon, 25 Jan 2016 20:04:20 +0100 In-Reply-To: <1453219845-30939-31-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 19 Jan 2016 09:10:38 -0700") Message-ID: <87bn891nwr.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v9 30/37] qapi: Canonicalize missing object to :empty List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, Michael Roth Eric Blake 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 > > --- > 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.