From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49512) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm0Ma-0004Gl-8S for qemu-devel@nongnu.org; Tue, 13 Oct 2015 10:15:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zm0MV-0005GV-HZ for qemu-devel@nongnu.org; Tue, 13 Oct 2015 10:15:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59388) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm0MV-0005GC-9V for qemu-devel@nongnu.org; Tue, 13 Oct 2015 10:15:31 -0400 References: <1444710158-8723-1-git-send-email-eblake@redhat.com> <1444710158-8723-11-git-send-email-eblake@redhat.com> <87fv1f7yvl.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <561D11F9.4010908@redhat.com> Date: Tue, 13 Oct 2015 08:15:21 -0600 MIME-Version: 1.0 In-Reply-To: <87fv1f7yvl.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="KwagjhKwqVmLWIwX0TFBBXL2Kgdxh2jlr" Subject: Re: [Qemu-devel] [PATCH v8 10/18] qapi: Move union tag quirks into subclass List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --KwagjhKwqVmLWIwX0TFBBXL2Kgdxh2jlr Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/13/2015 06:10 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Right now, simple unions have a quirk of using 'kind' in the C >> struct to match the QMP wire name 'type'. This has resulted in >> messy clients each doing special cases. While we plan to >> eventually rename things to match, it is better in the meantime >> to consolidate the quirks into a special subclass, by adding a >> new member.c_name() function. This will also make it easier >> for reworking how alternate types are laid out in a future >> patch. Use the new c_name() function where possible. >=20 > Terminology: "the new c_name() method". >=20 > I don't like having both function c_name() and method c_name(), because= > it's very easy to use the function when you should use the method, and > the mistakes will make things break only rarely, so they can go > undetected easily. I'm okay with this patch only because you assure me= > the whole thing is temporary: "# TODO Drop this class once we no longer= > have the 'type'/'kind' mismatch". Hmm. Even after my type/kind fix is in, my local tree doesn't (yet) remove uses of c_name(), because of its shorter typing. But you are correct that once the rename is in and the temporary QAPISchemaObjectTypeUnionTag is deleted, then there is no longer a difference between member.c_name() and the longer c_name(member.name). On the other hand, my patch subset C adds a member.c_type() function which is required for use on simplified alternate layout, because it is not always the same as member.type.c_type() or c_type(member.type.name). As it is, we already have cases where c_name(entity.name) is not the same as entity.c_name(), so we already have the confusion of when to use the global function vs. the member function. Is it worth renaming things so that the global function and member functions have different names? If so, which one would be renamed, and to what? >=20 >> No change to generated code. >> >> Signed-off-by: Eric Blake >> >> --- >=20 > v8: use + instead of interpolation in a few places, rebase to simpler > .is_implicit(), use .c_name() more. Whoops, forgot to 'git commit --amend' this one. Looks like you are also viewing interdiffs, though, which makes me feel better about how the series is progressing. >> +++ b/scripts/qapi-commands.py >> @@ -32,8 +32,8 @@ def gen_call(name, arg_type, ret_type): >> if arg_type: >> for memb in arg_type.members: >> if memb.optional: >> - argstr +=3D 'has_%s, ' % c_name(memb.name) >> - argstr +=3D '%s, ' % c_name(memb.name) >> + argstr +=3D 'has_' + memb.c_name() + ', ' >> + argstr +=3D memb.c_name() + ', ' >=20 > I like to use + instead of % in sufficiently simple cases myself. Not > sure this is one. See also change to gen_params() below. >> @@ -1555,8 +1567,8 @@ def gen_params(arg_type, extra): >> ret +=3D sep >> sep =3D ', ' >> if memb.optional: >> - ret +=3D 'bool has_%s, ' % c_name(memb.name) >> - ret +=3D '%s %s' % (memb.type.c_type(is_param=3DTrue), c_name= (memb.name)) >> + ret +=3D 'bool has_' + memb.c_name() + sep >> + ret +=3D '%s %s' % (memb.type.c_type(is_param=3DTrue), memb.c= _name()) >> if extra: >> ret +=3D sep + extra >> return ret >=20 > New hunk in v8, to remain consistent with gen_call(). >=20 > I doubt replacing literal ', ' by sep is making things clearer. Fair enough - if there is reason for respin, I can undo the changes to using '+'. >> @@ -1604,7 +1616,7 @@ def gen_visit_fields(members, prefix=3D'', need_= cast=3DFalse, skiperr=3DFalse): >> visit_type_%(c_type)s(v, %(cast)s&%(prefix)s%(c_name)s, "%(name)s= ", %(errp)s); >> ''', >> c_type=3Dmemb.type.c_name(), prefix=3Dprefix, ca= st=3Dcast, >> - c_name=3Dc_name(memb.name), name=3Dmemb.name, >> + c_name=3Dmemb.c_name(), name=3Dmemb.name, >> errp=3Derrparg) >> ret +=3D gen_err_check(skiperr=3Dskiperr) >=20 > New hunks in v8. Do you change from function c_name() to method > .c_name() Just for consistency, or is there a more serious reason? It matters the most in qapi-type's gen_union(); everywhere else, it is just for consistency and less typing. What is the plan of attack on this one - will I need to respin a v9? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --KwagjhKwqVmLWIwX0TFBBXL2Kgdxh2jlr 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/ iQEcBAEBCAAGBQJWHRH5AAoJEKeha0olJ0NqvEAIAIjnrBN7sczl/12vpFactWsF qXeViSfeZXGV/VKhprSMpA+C1oOswX7ZlHIWUH/O7WjlAIjajYBcuiNTlJuVmJTo k7fUV0DmhdagO2DMsNp+KbEjXGeh601N9pQMme0teV51SH06olFulhujU2qWQ8F5 bzmugH1jM3o0aMuua8jZnEZbPH0LR8g0hRIaKehgBSfIsSyksL1QKYYRFL6pPsjv XiW8zmt/0CdK0QYXppp9cjZLyO62IZ+/syxr4XPYJnC66SIOzt0wDV51u7j5GiTA nWz7CIVEhv+DvyVOv3m0EI0e/ZE69s7LUTT6sxlEszjfU2UNf71QTxUJI/HFZw0= =oJmy -----END PGP SIGNATURE----- --KwagjhKwqVmLWIwX0TFBBXL2Kgdxh2jlr--