From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40005) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHbZj-00066u-Te for qemu-devel@nongnu.org; Tue, 21 Jul 2015 13:43:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZHbZg-0002Zf-M9 for qemu-devel@nongnu.org; Tue, 21 Jul 2015 13:43:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44170) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHbZg-0002ZY-Dz for qemu-devel@nongnu.org; Tue, 21 Jul 2015 13:43:28 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-20-git-send-email-armbru@redhat.com> From: Eric Blake Message-ID: <55AE84BA.3070807@redhat.com> Date: Tue, 21 Jul 2015 11:43:22 -0600 MIME-Version: 1.0 In-Reply-To: <1435782155-31412-20-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="sNnBUGVn3hsfN9B8MCJ1QoPMflRBMQcWr" Subject: Re: [Qemu-devel] [PATCH RFC v2 19/47] qapi: Generated code cleanup 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) --sNnBUGVn3hsfN9B8MCJ1QoPMflRBMQcWr Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/01/2015 02:22 PM, Markus Armbruster wrote: > Clean up white-space, brace placement, and superfluous Incomplete sentence. I bet it's because your editor line-wrapped, and the rest of your sentence was something like '#ifndef in a .c file.' (see [1] below), then git ate the line thinking it was a comment. I think I'm okay with cramming all of these cleanups into one patch rather than trying to do one style of cleanup per patch (blank lines, { placement, and guard cleanup), but the commit message could do a better job of explaining things. >=20 > Signed-off-by: Markus Armbruster > --- > scripts/qapi-commands.py | 1 + > scripts/qapi-event.py | 3 +-- > scripts/qapi-types.py | 66 +++++++++++++++++++++++-----------------= -------- > scripts/qapi-visit.py | 1 + > 4 files changed, 34 insertions(+), 37 deletions(-) Missing changes to docs/qapi-code-gen.txt to reflect the improvements. Since having docs that are stale compared to implementation can be misleading, I'd like to wait for v2 before giving my R-b; but see also [2] below. Overall, I'm a fan of the cleanups. I'm going to intersperse diffs of the generated files that were caused by some of these changes: >=20 > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index cfbd59c..223216d 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -222,6 +222,7 @@ def gen_marshal_input(name, args, ret_type, middle_= mode): > ret +=3D mcgen(''' > =20 > (void)args; > + > ''') This and similar hunts avoids extra blank lines. Not worth showing a diff to the generated files. > =20 > ret +=3D gen_sync_call(name, args, ret_type) > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > index 88b0620..7f238df 100644 > --- a/scripts/qapi-event.py > +++ b/scripts/qapi-event.py > @@ -167,8 +167,7 @@ extern const char *%(event_enum_name)s_lookup[]; > event_enum_name =3D event_enum_name) > =20 > enum_decl =3D mcgen(''' > -typedef enum %(event_enum_name)s > -{ > +typedef enum %(event_enum_name)s { Several hunks like this; gets us generally closer to qemu style; with generated diffs like: qapi-types.h: -typedef struct int32List -{ +typedef struct int32List { union { int32_t value; uint64_t padding; > @@ -105,7 +103,8 @@ struct %(name)s > =20 > def generate_enum_lookup(name, values): > ret =3D mcgen(''' > -const char * const %(name)s_lookup[] =3D { > + > +const char *const %(name)s_lookup[] =3D { [2] generated diffs like this: qapi-types.c: -const char * const OnOffAuto_lookup[] =3D { +const char *const OnOffAuto_lookup[] =3D { Hmm - we already failed to update docs/qapi-code-gen.txt in the past; we added a const in commit 2e4450ff that is missing from the documentation. > @@ -335,22 +333,22 @@ for typename in builtin_types.keys(): > fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL")) > =20 > for expr in exprs: > - ret =3D "\n" > + 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']) + "\n" > + 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'])) More work at avoiding extra blank lines. > @@ -370,34 +368,32 @@ fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_= DECL")) > # have the functions defined, so we use -b option to provide control > # over these cases > if do_builtins: > - fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF")) > for typename in builtin_types.keys(): > fdef.write(generate_type_cleanup(typename + "List")) > - fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF")) [1] this is the hunk whose description got corrupted in your commit message. It gives a diff like this: qapi-types.c: - -#ifndef QAPI_TYPES_BUILTIN_CLEANUP_DEF -#define QAPI_TYPES_BUILTIN_CLEANUP_DEF - - void qapi_free_int32List(int32List *obj) We conditionally declare the functions in the .h, but the .c is not compiled multiple times, so there is no need for a header-style guard. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --sNnBUGVn3hsfN9B8MCJ1QoPMflRBMQcWr 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/ iQEcBAEBCAAGBQJVroS6AAoJEKeha0olJ0NqwYwH/jBuRnw9GDstZLWClbb3Nclo 9B/IOyEsqWsXMaLbhnaPPLlN5ezc0gipc+EFaKWkvxHeoF4Ogm7GPcipBZektiEG 2+sfyrUzuSV3TcmcOPIbmWM2YOxlbxfoWeRWsxgYByFxZ0N8X7u50y/2aYkchHb2 GnN6POyK/eFPCHn9o4OoWgvAfnmu8Xz4EM48/yvxm/rDyyh4uCWQu/Fg9OSaNcd8 AWu33TiBo2m7X9JrZIXzkEkz9+fc0jM7Zg/87atJIB711EnF2yqSK52Am/tyNk8/ dsudoI7qOOtkFIYtsZvl+bRSmM9BmJJC05+J/nrOLRjbl4/CQOwgGMwEeXSjv2k= =eFWF -----END PGP SIGNATURE----- --sNnBUGVn3hsfN9B8MCJ1QoPMflRBMQcWr--