From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39821) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpGZ2-0007eH-Ln for qemu-devel@nongnu.org; Thu, 22 Oct 2015 10:10:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZpGYx-0002zM-Lv for qemu-devel@nongnu.org; Thu, 22 Oct 2015 10:09:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36977) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpGYx-0002zH-Eh for qemu-devel@nongnu.org; Thu, 22 Oct 2015 10:09:51 -0400 References: <1444968943-11254-1-git-send-email-eblake@redhat.com> <1444968943-11254-8-git-send-email-eblake@redhat.com> <87611z100f.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <5628EE28.8070003@redhat.com> Date: Thu, 22 Oct 2015 08:09:44 -0600 MIME-Version: 1.0 In-Reply-To: <87611z100f.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="DbGkJSRfFju37GuWFEop7N39QwACTXoPS" Subject: Re: [Qemu-devel] [PATCH v9 07/17] qapi: Start converting to new qapi union layout 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) --DbGkJSRfFju37GuWFEop7N39QwACTXoPS Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/22/2015 07:54 AM, Markus Armbruster wrote: >=20 > This is clever and ugly in equal measure. I respect that. Fortunately= , > it's also temporary. >=20 >> Flat unions do not need the anonymous union for the tag member, >> as we already fixed that to use the member name instead of 'kind' >> back in commit 0f61af3e. >=20 > Unlike then, we need multiple commits for simple unions, because they'r= e > more widely used? Yes. In fact, both you and I expressed surprise back then that the main body of qemu didn't need adjusting - our only use of flat unions was hidden behind QDict manipulations rather than direct generated qapi struct, explaining why nothing was affected when we converted flat unions. But a useful note for the commit message at any rate. >=20 >> On the other hand, the duplication >> means that we temporarily cannot support 'u' as a branch name. >=20 > Separate paragraph, because now you're talking about the *other* > anonymous union. >=20 >> Later, when the conversions are complete, we will remove the >> duplication hacks and restore support for 'u' as a branch name. And based on comments on 3/17, I'm deferring any testsuite changes related to 'u' collisions until after this conversion to inline base is complete, so this part of the commit message actually disappears in v10 because I'm no longer touching qapi-schema-test this early. >> >> Note, however, that we do not rename the generated enum, which >> is still 'FooKind'. A further patch could generate implicit >> enums as 'FooType', but that causes more churn to C code, and >> gets harder since the generator already reserved the '*Kind' >> namespace, but there are already QMP constructs with '*Type' >> naming which means we cannot easily reserve it for qapi. >=20 > Oh, we can reserve whatever we want in QAPI, it's just a lot of churn t= o > adapt the QAPI-using code. >=20 > I'd simply say "but that would cause substantial churn to C code, as > there are already QAPI definitions with '*Type' naming". Okay. >> for var in variants.variants: >> # Ugly special case for simple union TODO get rid of it >> typ =3D var.simple_union_type() or var.type >> - ret +=3D mcgen(''' >> + tmp +=3D mcgen(''' >> %(c_type)s %(c_name)s; >> ''', >> c_type=3Dtyp.c_type(), >> c_name=3Dc_name(var.name)) >> >> + ret +=3D tmp >> + ret +=3D ' ' + '\n '.join(tmp.split('\n')) >> ret +=3D mcgen(''' >> + } u; >> }; >> }; >> ''') >=20 > It took me some head-scratching to understand why this generates > correctly indented output. If it wasn't temporary code, I'd ask for > cleanup. Would a comment help? It's because we add 4 spaces after each newline, but need an indent prior to the first line of tmp, and the '} u;' line picks up four spaces after the last line of tmp. >=20 > Second part: convert qapi-visit.py. Not mentioned in commit message. > Separate patch, perhaps? Sure, I could split. >=20 > Third part: work around temporary clash with 'u'. Needs to remain in > this patch, obviously. Suggest to amend the commit message to say >=20 > On the other hand, the duplication means that we temporarily cannot= > support 'u' as a branch name. Adapt a few tests that do. Or, rather, dropped entirely, because the tests for collisions with 'u' will be deferred until after the conversion is complete. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --DbGkJSRfFju37GuWFEop7N39QwACTXoPS 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/ iQEcBAEBCAAGBQJWKO4pAAoJEKeha0olJ0NqtMsH/1qd+1NDmhDQDN5MIgI/9ET8 SM36FPDbO6I5vFcrD+nJL66ByWEwufMw4lGmWdCgNuQVL1FJqcyfpGO7wFNHVEwt i0E+krKfxcyBjt9UMEOCZMRq0svGixRXneDEzqmc/+dpS8VY/ZJ6821rz2XKuRF6 SJg3e+IqsBK+6LZGWbvCZlaUx82LVSLcuT/r/GvScdgrpPfDuhNo5xG6LXFcQqkf a7v+iQlvrROHKiorACTv1yUv4MsoddrL31A7Qb5e90cY0SDQxCMH6eITr8829h3U TnCcsT/nd2NGnVis0r+Rl1gk08xGeF9gGKOEmj5GcBbYa3UCdkYsWHE2mIsT/pU= =XHKO -----END PGP SIGNATURE----- --DbGkJSRfFju37GuWFEop7N39QwACTXoPS--