From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55997) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZIJjV-0006K7-FF for qemu-devel@nongnu.org; Thu, 23 Jul 2015 12:52:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZIJjO-00060Y-I6 for qemu-devel@nongnu.org; Thu, 23 Jul 2015 12:52:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50894) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZIJjO-00060J-AN for qemu-devel@nongnu.org; Thu, 23 Jul 2015 12:52:26 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-34-git-send-email-armbru@redhat.com> From: Eric Blake Message-ID: <55B11AF1.3000509@redhat.com> Date: Thu, 23 Jul 2015 10:48:49 -0600 MIME-Version: 1.0 In-Reply-To: <1435782155-31412-34-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="u6PQSVe4L4SSDXUADuFuJiHcIefBMfSTK" Subject: Re: [Qemu-devel] [PATCH RFC v2 33/47] qapi: Clean up after recent conversions to QAPISchemaVisitor 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) --u6PQSVe4L4SSDXUADuFuJiHcIefBMfSTK Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/01/2015 02:22 PM, Markus Armbruster wrote: > Drop helper functions that are now unused. >=20 > Make pylint reasonably happy. >=20 > Rename generate_FOO() functions to gen_FOO() for consistency. >=20 > Use more consistent and sensible variable names. >=20 > Consistently use c_ for mapping keys when their value is a C > identifier or type. >=20 > Simplify gen_enum() and gen_visit_union() >=20 > Consistently use single quotes for C text string literals. Not sure if it is worth splitting this into pieces. Fortunately, there are no changes to generated output. >=20 > Signed-off-by: Markus Armbruster > --- > scripts/qapi-commands.py | 109 +++++++++++++++++++------------------ > scripts/qapi-event.py | 117 +++++++++++++++++++--------------------= - > scripts/qapi-types.py | 68 ++++++++++++----------- > scripts/qapi-visit.py | 121 ++++++++++++++++++++-------------------= -- > scripts/qapi.py | 138 +++++++++------------------------------= -------- > 5 files changed, 229 insertions(+), 324 deletions(-) >=20 > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index d3bddb6..5d11032 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -15,20 +15,20 @@ > from qapi import * > import re > =20 > -def generate_command_decl(name, args, ret_type): > - arglist=3D"" > +def gen_command_decl(name, args, rets): I can see how 'args' is plural (even if it is a single string for the name of a type containing the args), but should it be 'ret' instead of 'rets'? > @@ -40,34 +40,37 @@ if (%(err)s) { > ''', > err=3Derr) > =20 > -def gen_sync_call(name, args, ret_type): > - ret =3D "" > - arglist=3D"" > - retval=3D"" > - if ret_type: > - retval =3D "retval =3D " > +def gen_call(name, args, rets): At least you're consistent on naming it 'rets', > + ret =3D '' and the naming lets you distinguish between the parameter (the type describing returned fields) and the local string (the generated C code holding the return information). > @@ -82,45 +76,46 @@ def generate_event_implement(api_name, event_name, = params): > =20 > + # Ugly: need to cast away the const > if memb.type.name =3D=3D "str": > - var_type =3D "(char **)" > + cast =3D "(char **)" And to think I called it out in a previous patch. So you noticed it too := ) Don't you want to use '(char **)' here, since it is a literal string destined for generated C? > else: > - var_type =3D "" > + cast =3D "" and '' here? > +++ b/scripts/qapi-types.py > @@ -16,22 +16,22 @@ from qapi import * > def gen_fwd_object_or_array(name): > return mcgen(''' > =20 > -typedef struct %(name)s %(name)s; > +typedef struct %(c_name)s %(c_name)s; > ''', > - name=3Dc_name(name)) > + c_name=3Dc_name(name)) > =20 > def gen_array(name, element_type): > return mcgen(''' > =20 > -struct %(name)s { > +struct %(c_name)s { > union { > %(c_type)s value; > uint64_t padding; > }; > - struct %(name)s *next; > + struct %(c_name)s *next; May be some churn here if you like my comment earlier in the series that this 'struct' is redundant. > +++ b/scripts/qapi-visit.py > -def generate_visit_struct_fields(name, members, base =3D None): > +def gen_visit_struct_fields(name, base, members): > struct_fields_seen.add(name) > - type=3Dbase.c_name(), c_name=3Dc_name('base')) > + c_type=3Dbase.c_name(), c_name=3Dc_name('base')) Possible churn here based on my earlier comments about c_name(constant) being constant. Fairly big, but aside from some minor '' quoting issues, I didn't see anything wrong. Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --u6PQSVe4L4SSDXUADuFuJiHcIefBMfSTK 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/ iQEcBAEBCAAGBQJVsRrxAAoJEKeha0olJ0NqwdUH+wR0e265HoccMaZUcerQ6R23 8e1wdjwE9+OAvRLLVCdkLN2Rbpnjs5/l9U07MvvBYgdIWD4CVlqc5qPw6WY3kdwq Yekp2mRy+re8aCExwAg946GYLj5lDmmaxWL0X2ubVfQxBcSHTm8s/O6yBU5OhTBs tf4WKv9f3VqudquPQqGfchqsPUCYnmAxlL0jHh0Hv9ERuR86+lqzPQIiTkZSSo29 erkO0QxZwjFqYbyM6eYow++/GiZTRGsFUsHLqZ/ZTJ90pZ+Si+8VmRBe7w+Vs02V tqzbAyQFf6lUpHC79Sgz1ddEbMLy9CxlGPus98mKj96TAk1kVvc0fOczIZXqllc= =Ro9+ -----END PGP SIGNATURE----- --u6PQSVe4L4SSDXUADuFuJiHcIefBMfSTK--