From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40151) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgvdF-0002wh-27 for qemu-devel@nongnu.org; Tue, 29 Sep 2015 10:11:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZgvdC-0000KM-TG for qemu-devel@nongnu.org; Tue, 29 Sep 2015 10:11:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50061) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgvdC-0000KA-Lr for qemu-devel@nongnu.org; Tue, 29 Sep 2015 10:11:46 -0400 References: <1443497249-15361-1-git-send-email-eblake@redhat.com> <1443497249-15361-6-git-send-email-eblake@redhat.com> <87a8s5l810.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <560A9C1C.1000903@redhat.com> Date: Tue, 29 Sep 2015 08:11:40 -0600 MIME-Version: 1.0 In-Reply-To: <87a8s5l810.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="L9jJK5cvhoxD8vdvKHvHICj7FMnIi4CA1" Subject: Re: [Qemu-devel] [PATCH v6 05/16] qapi: Test for various name collisions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --L9jJK5cvhoxD8vdvKHvHICj7FMnIi4CA1 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/29/2015 06:33 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Expose some weaknesses in the generator: we don't always forbid >> the generation of structs that contain multiple members that map >> to the same C name. >=20 > C or QMP name, perhaps? Lots of good advice for an improved commit message and test comments; looks like I'll do a v7 respin to address them. >=20 >> Oddly enough, while commit 1e6c1616 fixed a type 2b collision >> with 'base', >=20 > What collided with 'base' before 1e6c1616 and not after? Uggh. I wrote that comment late at night, and it shows. I was mixing up struct bases (which are boxed behind a 'FOO *base' member) and flat unions; but flat union base classes have always been expressed on a per-member basis. I'll either drop this paragraph entirely, or come up with a real example (about the only one that might be left is when we dropped the mis-use of 'kind', we were then introducing the possibility of collision with the 'discriminator':'name' - and that's one of the two tests added in this commit where we assert). >=20 >> it introduced a different collision that was not >> previously possible: now that the base members are no longer >> boxed behind 'base', they can collide with the enum tag members >> (used as member names within the embedded C union). >> >> Ultimately, if we need to have a flat union where an enum >> value clashes with a base member name, we could change the >=20 > Suggest "an enum tag member", because this is confusing enough without > using multiple names for the same thing :) >=20 >> generator to use something like '_tag_value' instead of >> 'value' for the C name of union members; >=20 > Or wrap the base in a struct, or give the union member a name. Giving the union member a name might be the least amount of typing, but would still touch quite a bit of code if we have to do it. >=20 >> but until such a >> need arises, it will probably be easier to just forbid these >> collisions. >=20 > Again, I'm not sure this makes sense to readers who aren't already > familiar with our name clashing issues. >=20 > Do you fix any of this later in your series? If yes, you could punt > detailed bug descriptions to the patches that fixes them. Yes, I fix a number of the issues later on (although only the two assertion failures get fixed in subset A; the rest of the fixups are in the tail end of v5 which will become subset B). >=20 >> Some of these collisions are already caught in the old-style >> parser checks, but ultimately we want all collisions to be >> caught in the new-style QAPISchema*.check() methods. >> >> Some of these negative tests will be deleted later, and positive >> tests added to qapi-schema-test.json in their place, when the >> generator code is reworked so that a particular collision no >> longer causes failed code generation. >> >> [git rename detection may be a bit confused by the actions of >> this commit, since it both creates and renames files that are >> equally small and similar in content] >=20 > Why square brackets? >=20 To delineate that this paragraph is is unrelated to the contents of the patch, but instead guides a reader to how to interpret the git diff of the patch - if rename detection is enabled, git botches the rename detection, doing things like: >> --- >> tests/Makefile | 10 +++++++= ++- >> .../{flat-union-branch-clash.out =3D> args-name-clash.err} | 0 claiming this file is a rename (in reality, I renamed flat-union-branch-clash.out to flat-union-clash-branch.out; and .out files never really rename to .err files other than the fact that both happened to be empty). We could drop it if desired, but I think it makes sense to keep the comment in qemu.git (because 'git diff' will botch the rename detection no matter how much in the future you are inspecting this patch). >> +++ b/tests/qapi-schema/args-name-clash.json >> @@ -0,0 +1,2 @@ >> +# FIXME - we should reject data with members that clash when mapped t= o C names >> +{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } } >=20 > Here, the collision is fairly obvious. We could spell it out anyway: >=20 > # Multiple members mapping to the same C identifier: both 'a-b' and > # 'a_b' map to a_b' > # FIXME they clash in generated C, should reject them cleanly instead. >=20 > Unfortunately, the test doesn't actually make the clash visible. Same > for all the other tests that produce clashes in C or QMP without > actually upsetting the generator. Oh well, good enough. And for every FIXME I add here, a later patch in the v5 series resolves it (either by making it a parse error, or by removing the generated collision). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --L9jJK5cvhoxD8vdvKHvHICj7FMnIi4CA1 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/ iQEcBAEBCAAGBQJWCpwcAAoJEKeha0olJ0NqPO4H/intGyr9M3aajbqU3QbklpM+ oiLCZ58BMZpOhcfj+yyGB2X9RrXd894aHtmW8ohiR6EXtYFKJYTEqGogxgXV0M1j +QGXDW4az9Cy71AD13ra/hjiweww53u25OXjm4wJW3kIG/TMruA0BCHLllO5xUeR o8QFB/ev7VBjMyBQAWakwryB02dRbAyKKtyFlvLcmmLjA+a/BCYstDKRvqaedOJp sGugbF7lt6L1kZnsPGU3KBIO0xHBIIqSjx2ycU7Vjm6a8Cc6ZVGE0uVRr+AWn+Ag BKoAcmrYZnkIoodMhIiz11J8CnFY95UgtKqKiIJeZIIM8Jk6EZd+4mGGqI5gseg= =5J2Q -----END PGP SIGNATURE----- --L9jJK5cvhoxD8vdvKHvHICj7FMnIi4CA1--