From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42984) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJneX-0000ca-V9 for qemu-devel@nongnu.org; Mon, 27 Jul 2015 15:01:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZJneT-0001cZ-R5 for qemu-devel@nongnu.org; Mon, 27 Jul 2015 15:01:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55694) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJneT-0001cU-KA for qemu-devel@nongnu.org; Mon, 27 Jul 2015 15:01:29 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-28-git-send-email-armbru@redhat.com> <55B01923.9050907@redhat.com> <87y4i1sd0u.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <55B68002.90400@redhat.com> Date: Mon, 27 Jul 2015 13:01:22 -0600 MIME-Version: 1.0 In-Reply-To: <87y4i1sd0u.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="j9CrpkK8kTVGWxeqCDdjO8kV74r62NbNI" Subject: Re: [Qemu-devel] [PATCH RFC v2 27/47] qapi-visit: Convert to QAPISchemaVisitor, fixing bugs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --j9CrpkK8kTVGWxeqCDdjO8kV74r62NbNI Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/27/2015 11:53 AM, Markus Armbruster wrote: >> Oh, and that means our generator has a collision bug that none of my >> added tests have exposed yet: you cannot have a base class and >> simultaneously add a member named 'base': >> >> { 'struct': 'Base', 'data': { 'i': 'int' } } >> { 'struct': 'Sub', 'base': 'Base', 'data': { 'base': 'str' } } >> >> because the generated C code is trying to use the name 'base' for its >> own purposes. >=20 > *sigh* >=20 >> I guess that means more pre-req patches to the series t= o >> expose the bug, and either tighten the parser to reject things for now= >> (easiest) or update the generator to not collide (harder, and fine for= a >> later series). >=20 > Yes. >=20 > Life would be easier if the original authors had adopted sane naming > conventions from the start. Like reserving ourselves a namespace based on single _ for internal use. We practically already have that - all user names either start with a letter or double underscore, so we could use single (and triple) underscore for internally-generated purposes, freeing up 'base', '*Kind', '*_MAX', and other namespace abuses back to the user. Well, we may also need to reserve mid-name double-underscore (that is, the user can only supply double underscore at the beginning, but not middle, of an identifier). Ah well, food for thought for later patches. >> Okay, I see where you are using .flat from the initial parse. I still= >> think it is a bit odd that you are defining '.flat' for each 'variant'= >> within 'variants', even though, for a given 'variants', all members wi= ll >> have the same setting of '.flat'. That makes me wonder if '.flat' >> should belong instead to the top-level 'variants' struct rather than t= o >> each 'variant' member. >=20 > Two reasons for putting flat where it is: >=20 > * The philosophical one: from the generator's point of view, there's no= > fundamental reason why all variants need to be flat or none. The > generator really doesn't care. And we may decide to exploit that down the road to allow some sort of qapi syntax for explicitly designating a union branch as flat or boxed, rather than the current approach of the type of union determining the status of all branch members. >=20 > * The pragmatic one (a.k.a. the real one): there are places where I use= > v.flat, but don't have the variants owning v handy. >=20 >> But again I wonder what would happen if you had instead normalized the= >> input of simple unions into always having an implicit struct (with >> single member 'data'), so that by the time you get here, you only have= >> to deal with a single representation of unions instead of having to >> still emit different things for flat vs. simple (since on the wire, we= >> already proved simple is shorthand that can be duplicated by a flat un= ion). >=20 > I hope we can get there! But at this stage of the conversion, I want t= o > minimize output change, and .flat makes preserving all its warts much > easier. Agreed. By the end of the series, I was convinced that the use of =2Eflat, at least in this series, makes sense. >>> + disc_key =3D variants.tag_member.name >>> + if not variants.tag_name: >>> + # we pointlessly use a different key for simple unions >> >> We could fix that (as a separate patch); wonder how much C code it wou= ld >> affect. A lot of these things that we can alter in generated code are= >> certainly easier to see now that we have a clean generator :) >=20 > Yup, the warts stand out now. And I've already demonstrated what sort of cleanups can be done to attack some of the warts: https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05266.html --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --j9CrpkK8kTVGWxeqCDdjO8kV74r62NbNI 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/ iQEcBAEBCAAGBQJVtoACAAoJEKeha0olJ0Nq2KgH/0Tzq5OgTwTvwod0EAxBcrtY QuiZQdQG0Zt1xUQYWZpzy677SEkB+9fg6gBwOT4niDEJRtvbuDWWXUmNqQWCyWaP PF7pd/5fRl7sjgxuJqm+la/5frKC9Khu+vF9rJ/+9s4tdx54x1SVh1T5NVq9K+u5 1gw4Uk0I7SpaJCMN4XExcYf4+lIukIwoSGVjG5k4UIOfmqe6QMPv+0V5NaeDsDpD NUEd0BFzRiIOP1hDIJEnh4+s3gT2mcqUIYZGiYwtjEzN4DQt0P1EemHShN8c9mBB yQoC5U3/WacPm3GsIo42uKqR0u0UReVI6e17u5jcX2cjGFKv1QBy0YOFuUlmHA0= =6zmS -----END PGP SIGNATURE----- --j9CrpkK8kTVGWxeqCDdjO8kV74r62NbNI--