From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59911) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHxv1-0006Mu-Is for qemu-devel@nongnu.org; Wed, 22 Jul 2015 13:35:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZHxux-0000sO-Ge for qemu-devel@nongnu.org; Wed, 22 Jul 2015 13:34:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58105) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHxux-0000rT-6Z for qemu-devel@nongnu.org; Wed, 22 Jul 2015 13:34:55 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-27-git-send-email-armbru@redhat.com> From: Eric Blake Message-ID: <55AFD438.2090303@redhat.com> Date: Wed, 22 Jul 2015 11:34:48 -0600 MIME-Version: 1.0 In-Reply-To: <1435782155-31412-27-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="tA9u7OnFs5aFiWPfK1O5OA8xGFSpSeMsP" Subject: Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, berto@igalia.com, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --tA9u7OnFs5aFiWPfK1O5OA8xGFSpSeMsP Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/01/2015 02:22 PM, Markus Armbruster wrote: > Fixes flat unions to get the base's base members. Test case is from > commit 2fc0043, in qapi-schema-test.json: >=20 >=20 > Flat union visitors remain broken. They'll be fixed next. Sadly, the generated files had a huge diffstat, making it very hard to determine if this change is sane: qapi-types.c | 1412 ++++++------- qapi-types.h | 3705 ++++++++++++++++++++---------------- qga/qapi-generated/qga-qapi-types.c | 110 - qga/qapi-generated/qga-qapi-types.h | 542 +++-- 4 files changed, 3208 insertions(+), 2561 deletions(-) At first, it looks easy, as in: qapi-types.c: +const int BlockdevRef_qtypes[QTYPE_MAX] =3D { =2E.. const char *const BlockdevRefKind_lookup[] =3D { =2E.. -const int BlockdevRef_qtypes[QTYPE_MAX] =3D { =2E.. (that is, the new visitor outputs the two arrays in a different order - I can live with that). Then it gets a bit crazy when using normal diff algorithms: -void qapi_free_int32List(int32List *obj) +void qapi_free_ACPIOSTInfo(ACPIOSTInfo *obj) where declarations are rearranged (so the previous commit, which attempted to sort declarations to make this one easier, did not quite succeed). Even in qapi-types.h things were rearranged: -#ifndef QAPI_TYPES_BUILTIN_STRUCT_DECL -#define QAPI_TYPES_BUILTIN_STRUCT_DECL +#ifndef QAPI_TYPES_BUILTIN +#define QAPI_TYPES_BUILTIN -typedef struct int32List { +typedef struct boolList boolList; + +struct boolList { I can understand the minor change in #ifdef name, and even the separation between typedef and struct declaration (even though the old approach of doing it all at once works). But the overall diff in the generated files would be easier to review if done in stages, and either make 25/47 sort closer to what this patch does, or add yet more prerequisite patches that further tweak sorting. Ideally, this patch will be a lot easier to review if the generated code is much closer to the pre-patch version (that is, separating sorting changes from other changes). On the other hand, yes, it's kind of a pain to patch old code to do things differently just before throwing it away with the new code in its place, so it becomes a judgment call of how confident we want to be that the new implementation isn't breaking things. I was able to make a bit more sense of things by using git's 'diff patience' algorithm, which showed things more as code motion rather than one-or-two-line changes to lots of common boilerplate, but even that diffstat is still big: qapi-types1.c | 2030 +++++++++++++++---------------- qapi-types1.h | 3707 +++++++++++++++++++++++++++++++++------------------------ 2 files changed, 3148 insertions(+), 2589 deletions(-) That said, I'll still at least review this code by inspection, and things still compile fine, so although I'm reluctant to give R-b while the patch is in RFC stage (because I didn't want to take the time to be certain the results are the same amidst so much churn), they are mostly sane. >=20 > Signed-off-by: Markus Armbruster > --- > scripts/qapi-types.py | 268 +++++++++++++-----------= -------- > tests/qapi-schema/qapi-schema-test.json | 4 +- > 2 files changed, 114 insertions(+), 158 deletions(-) >=20 > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index a48ad9c..d6185c6 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -2,88 +2,67 @@ > # QAPI types generator > # > # Copyright IBM, Corp. 2011 > +# Copyright (c) 2013-2015 Red Hat Inc. > # > # Authors: > # Anthony Liguori > +# Markus Armbruster > # > # This work is licensed under the terms of the GNU GPL, version 2. > # See the COPYING file in the top-level directory. > =20 > -from ordereddict import OrderedDict > from qapi import * > =20 > -def generate_fwd_builtin(name): > - return mcgen(''' > - > -typedef struct %(name)sList { For example, the change of splitting this into: typedef struct %(name)sList; struct %(name)sList { done as a separate patch would make the generated diff of this patch smaller. > =20 > -def generate_struct_fields(members): > +def gen_struct_field(name, typ, optional): > ret =3D '' > =20 > - for argname, argentry, optional in parse_args(members): > - if optional: > - ret +=3D mcgen(''' > + if optional: > + ret +=3D mcgen(''' (sometimes it's annoying that python requires indentation changes when changing control flow; for C files, sometimes I split a patch that merely adds {} and indentation from another patch that changes logic, so that the logic change isn't drowned by the reindentation) > +class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): > + def __init__(self): > + self.decl =3D None > + self.defn =3D None > + self.fwdecl =3D None > + self.fwdefn =3D None > + self.btin =3D None > + def visit_begin(self): > + self.decl =3D '' > + self.defn =3D '' > + self.fwdecl =3D '' > + self.fwdefn =3D '' > + self.btin =3D guardstart('QAPI_TYPES_BUILTIN') > + def visit_end(self): > + self.decl =3D self.fwdecl + self.decl > + self.fwdecl =3D None > + self.defn =3D self.fwdefn + self.defn > + self.fwdefn =3D None > + # To avoid header dependency hell, we always generate > + # declarations for built-in types in our header files and > + # simply guard them. > + self.btin +=3D guardend('QAPI_TYPES_BUILTIN') > + self.decl =3D self.btin + self.decl > + self.btin =3D None > + # Doesn't work for cases where we link in multiple objects > + # that have the functions defined, so generate them only with > + # option -b (do_builtins). Does this comment... > + def _gen_type_cleanup(self, name): > + self.decl +=3D generate_type_cleanup_decl(name) > + self.defn +=3D generate_type_cleanup(name) > + def visit_enum_type(self, name, info, values): > + self.fwdecl +=3D generate_enum(name, values) > + self.fwdefn +=3D generate_enum_lookup(name, values) > + def visit_array_type(self, name, info, element_type): > + if isinstance(element_type, QAPISchemaBuiltinType): > + self.btin +=3D gen_fwd_object_or_array(name) > + self.btin +=3D gen_array(name, element_type) > + self.btin +=3D generate_type_cleanup_decl(name) > + if do_builtins: > + self.defn +=3D generate_type_cleanup(name) =2E..fit better here? > + else: > + self.fwdecl +=3D gen_fwd_object_or_array(name) > + self.decl +=3D gen_array(name, element_type) > + self._gen_type_cleanup(name) > + def visit_object_type(self, name, info, base, members, variants): > + if info: So to make sure I understand, this ensures that implicit types (those handled merely by parameters in a function or as members of a struct) have no declarations required in the -types files. > + self.fwdecl +=3D gen_fwd_object_or_array(name) > + if variants: > + self.decl +=3D gen_union(name, base, variants) Is it worth 'assert not members' at this point? > + else: > + self.decl +=3D gen_struct(name, base, members) > + self._gen_type_cleanup(name) > + def visit_alternate_type(self, name, info, variants): > + self.fwdecl +=3D gen_fwd_object_or_array(name) > + self.fwdefn +=3D gen_alternate_qtypes(name, variants) > + self.decl +=3D gen_union(name, None, variants) > + self.decl +=3D gen_alternate_qtypes_decl(name) > + self._gen_type_cleanup(name) > + The visitor looks reasonable from inspection review. > do_builtins =3D False > =20 > (input_file, output_dir, do_c, do_h, prefix, opts) =3D \ > @@ -325,77 +348,10 @@ fdecl.write(mcgen(''' > #include > ''')) > =20 > -exprs =3D QAPISchema(input_file).get_exprs() > - > -fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL")) > -for typename in builtin_types.keys(): > - fdecl.write(generate_fwd_builtin(typename)) > -fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL")) This is a place where adding sorting in 25/47 (or a separate patch) would make the impact on generated code from this patch smaller. > - > -for expr in exprs: > - ret =3D "" > - if expr.has_key('struct'): > - ret +=3D generate_fwd_struct(expr['struct']) > - elif expr.has_key('enum'): > - ret +=3D generate_enum(expr['enum'], expr['data']) > - ret +=3D generate_fwd_enum_struct(expr['enum']) > - fdef.write(generate_enum_lookup(expr['enum'], expr['data'])) > - elif expr.has_key('union'): > - ret +=3D generate_fwd_struct(expr['union']) > - enum_define =3D discriminator_find_enum_define(expr) > - if not enum_define: > - ret +=3D generate_enum('%sKind' % expr['union'], expr['dat= a'].keys()) > - fdef.write(generate_enum_lookup('%sKind' % expr['union'], > - expr['data'].keys())) > - elif expr.has_key('alternate'): > - ret +=3D generate_fwd_struct(expr['alternate']) > - ret +=3D generate_enum('%sKind' % expr['alternate'], expr['dat= a'].keys()) > - fdef.write(generate_enum_lookup('%sKind' % expr['alternate'], > - expr['data'].keys())) Tweaking the order of _qtypes[] vs. _lookup[] here in a pre-req patch would also help review of the diff generated by this patch. > - fdef.write(generate_alternate_qtypes(expr)) > - else: > - continue > - fdecl.write(ret) > - > -# to avoid header dependency hell, we always generate declarations > -# for built-in types in our header files and simply guard them > -fdecl.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DECL")) > -for typename in builtin_types.keys(): > - fdecl.write(generate_type_cleanup_decl(typename + "List")) > -fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL")) > - > -# ...this doesn't work for cases where we link in multiple objects tha= t > -# have the functions defined, so we use -b option to provide control > -# over these cases > -if do_builtins: > - for typename in builtin_types.keys(): > - fdef.write(generate_type_cleanup(typename + "List")) Another iteration worth sorting in an earlier patch. > - > -for expr in exprs: > - ret =3D "" > - if expr.has_key('struct'): > - ret +=3D generate_struct(expr) + "\n" > - ret +=3D generate_type_cleanup_decl(expr['struct'] + "List") > - fdef.write(generate_type_cleanup(expr['struct'] + "List")) > - ret +=3D generate_type_cleanup_decl(expr['struct']) > - fdef.write(generate_type_cleanup(expr['struct'])) I think part of the large size of the generated diff comes from whether array fooList types are emitted in a different order via the visitor. > - elif expr.has_key('union'): > - ret +=3D generate_union(expr, 'union') + "\n" > - ret +=3D generate_type_cleanup_decl(expr['union'] + "List") > - fdef.write(generate_type_cleanup(expr['union'] + "List")) > - ret +=3D generate_type_cleanup_decl(expr['union']) > - fdef.write(generate_type_cleanup(expr['union'])) > - elif expr.has_key('alternate'): > - ret +=3D generate_union(expr, 'alternate') + "\n" > - ret +=3D generate_type_cleanup_decl(expr['alternate'] + "List"= ) > - fdef.write(generate_type_cleanup(expr['alternate'] + "List")) > - ret +=3D generate_type_cleanup_decl(expr['alternate']) > - fdef.write(generate_type_cleanup(expr['alternate'])) > - elif expr.has_key('enum'): > - ret +=3D "\n" + generate_type_cleanup_decl(expr['enum'] + "Lis= t") > - fdef.write(generate_type_cleanup(expr['enum'] + "List")) > - else: > - continue > - fdecl.write(ret) > +schema =3D QAPISchema(input_file) > +gen =3D QAPISchemaGenTypeVisitor() > +schema.visit(gen) > +fdef.write(gen.defn) > +fdecl.write(gen.decl) > =20 > close_output(fdef, fdecl) > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schem= a/qapi-schema-test.json > index a9e5aab..257b4d4 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -39,8 +39,8 @@ > 'data': { 'value1' : 'UserDefA', > 'value2' : 'UserDefB', > 'value3' : 'UserDefB' } } > -# FIXME generated struct UserDefFlatUnion has members for direct base > -# UserDefUnionBase, but lacks members for indirect base UserDefZero > +# FIXME generated visit_type_UserDefFlatUnion_fields() fails to visit > +# members of indirect base UserDefZero > =20 > { 'struct': 'UserDefUnionBase', > 'base': 'UserDefZero', >=20 Overall, moving in the right direction. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --tA9u7OnFs5aFiWPfK1O5OA8xGFSpSeMsP Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJVr9Q4AAoJEKeha0olJ0Nq/tsIAJgv1Mp4lDkrKf1e7YoC261w sUQ0p3o258qQ7EQ6pdalbkq21GxA7GwtGDcF/4k6gVfMj6MoqagDfxImeHjQtq4w HQLz2FzsKGkBvezcoBgcHgNQ6/aLcgB7G9RLp4k88/9qz/q5d/jr+ZdR6WBUdGP/ aGwpxgw6Ll/Jy7WEgaX6styUzCrOVFYTr3jkZE4fTUSKF7bdd7WHIFZvGGKruwrs N4FRQJeS0FOvQeVROKXq0LLIQJgLbY+ZZ9IdhOQal6RFTB20AbX0uZdAsoykkKIh dkiD9XjXvDhCvIXTYRFKXUfD7x2DCntqEMpUafrzVM9xih4/HhOJkPb3Y/CxNZI= =sYON -----END PGP SIGNATURE----- --tA9u7OnFs5aFiWPfK1O5OA8xGFSpSeMsP--