From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJiyP-0005HL-NF for qemu-devel@nongnu.org; Mon, 27 Jul 2015 10:01:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZJiyL-0001VO-7i for qemu-devel@nongnu.org; Mon, 27 Jul 2015 10:01:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46927) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJiyK-0001VK-Vx for qemu-devel@nongnu.org; Mon, 27 Jul 2015 10:01:41 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-22-git-send-email-armbru@redhat.com> <55AEAC64.1090202@redhat.com> <87oaiyq7i0.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <55B639BE.3010607@redhat.com> Date: Mon, 27 Jul 2015 08:01:34 -0600 MIME-Version: 1.0 In-Reply-To: <87oaiyq7i0.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="L0iGQk1tCLkUCpPVMgkhasO0vHG8G19IK" Subject: Re: [Qemu-devel] [PATCH RFC v2 21/47] qapi: New QAPISchema intermediate reperesentation 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) --L0iGQk1tCLkUCpPVMgkhasO0vHG8G19IK Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/27/2015 03:23 AM, Markus Armbruster wrote: >>> >>> Create a bunch of classes to represent QAPI schemata. >>> >>> Have the QAPISchema initializer call the parser, then walk the syntax= >>> tree to create the new internal representation, and finally perform >>> semantic analysis. >>> >>> +class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): >>> + def __init__(self, name, typ, flat): >>> + QAPISchemaObjectTypeMember.__init__(self, name, typ, False) >>> + assert isinstance(flat, bool) >>> + self.flat =3D flat >>> + def check(self, schema, tag_type, seen): >>> + QAPISchemaObjectTypeMember.check(self, schema, [], seen) >>> + assert self.name in tag_type.values >>> + if self.flat: >>> + self.type.check(schema) >>> + assert isinstance(self.type, QAPISchemaObjectType) >> >> Do we really want to be tracking self.flat for each variant? After al= l, >> docs/qapi-code-gen.txt already describes the mapping from simple union= >> into flat union (as if the flat union had a base class with single >> member 'kind' of the right type, then each branch of the union compose= d >> of an implicit object with a lone member 'data' of the correct type). >> In other words, is it any better to just normalize into that form now,= >> such that each QAPISchemaObjectTypeVariant is merely a (often >> one-element) list of name:type members being added to the overall >> QAPISchemaObject? >=20 > I tried to do exactly that, but got bogged down in special cases and > copped out. Then I went on vacation, and now I don't remember the exac= t > problems anymore %-} >=20 > I guess / hope it's just relatively pointless differences in the > generated C code I didn't want to get rid of at this time. The series > is long and hairy enough as it is... >=20 >> But I guess it remains to be seen how you use >> self.flat before knowing if it is worth normalizing away from it. >=20 > At least introspect.json is oblivious of it. Yeah, that was my conclusion by the end of the series - we still had enough special cases where we generate different code for simple unions than for flat unions. It would be possible to merge the generator and simplify things further, but at this point, it is best done as a follow-up series because it will touch lots of C code (anything that uses a simple union would have to convert to the common representation). And you actually did reference .flat in the patch that first added qapi-introspect.py (good that it did not leak to the introspection output, but it did have to be considered when figuring out what to output); again, something you may want to rework before polishing this into v3, or something you may end up just documenting as a possible cleanup for a future series. But after having reviewed the whole series now, I'm able to live with your use of .flat, if only because it makes the initial conversion faster. >>> + >>> + def lookup_entity(self, name, typ=3DNone): >>> + ent =3D self.entity_dict.get(name) >>> + if typ and not isinstance(ent, typ): >>> + return None >>> + return ent >> >> Ah, so you enforce a shared namespace between types, commands, and >> events (all three are in self.entity_dict), but can use the typ >> parameter to allow limiting a lookup to just types. >=20 > Yes. It's a convenience feature. Documentation comments never hurt :) (You did mention in the cover letter that part of the reason this was still RFC was that it was lacking documentation) --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --L0iGQk1tCLkUCpPVMgkhasO0vHG8G19IK 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/ iQEcBAEBCAAGBQJVtjm+AAoJEKeha0olJ0NqFPEH/3M9MIkrxmxHvWYkVDT2f4SG +lXCgBvVpl1ckTrR7VVHEZPs0b+i+CCXeJuSQFvHsYwkS+IH3tsseOyg5xR4MdLr XJ/irAR1ZEm9/HiayT6RJCsbPR4L67hOOS3+3UzuLqvVPOXepLEWLwk2UJUTDFm2 I6kLKPflOhpeTRYKdpjnXBRQ0TKbJzVA2rAMVOXNOFxtNvFNiRhia1IrPCWyQ8rM PUwsv8kp01CHNFIUc6Y+v5lokw8ZLGWZ8gfzGb4n6AKFR0SkiHzsLcT61Nsr2I3x 62cs8TT0w6femUPKFoXcW34D46WUsX5zBgI/Isr5HYelDU11TaQeH/qyWVqAvUA= =UcYG -----END PGP SIGNATURE----- --L0iGQk1tCLkUCpPVMgkhasO0vHG8G19IK--