From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40024) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbaDv-0002jw-Fb for qemu-devel@nongnu.org; Fri, 27 Mar 2015 15:47:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbaDn-0005S9-V0 for qemu-devel@nongnu.org; Fri, 27 Mar 2015 15:47:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58121) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbaDn-0005Ru-NW for qemu-devel@nongnu.org; Fri, 27 Mar 2015 15:47:11 -0400 Message-ID: <5515B3BD.6060909@redhat.com> Date: Fri, 27 Mar 2015 13:47:09 -0600 From: Eric Blake MIME-Version: 1.0 References: <1427227433-5030-1-git-send-email-eblake@redhat.com> <1427227433-5030-7-git-send-email-eblake@redhat.com> <87h9t76gng.fsf@blackfin.pond.sub.org> <55142006.50606@redhat.com> <87fv8qlj1e.fsf@blackfin.pond.sub.org> In-Reply-To: <87fv8qlj1e.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="1itXx1Q1VRC5Q37QdKXDulJvWkKjrdBAo" Subject: Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, wenchaoqemu@gmail.com, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --1itXx1Q1VRC5Q37QdKXDulJvWkKjrdBAo Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 03/27/2015 06:30 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> On 03/26/2015 07:18 AM, Markus Armbruster wrote: >>> Eric Blake writes: >>> >>>> Demonstrate that the qapi generator doesn't deal well with unions >>>> that aren't up to par. Later patches will update the expected >>>> reseults as the generator is made stricter. Oh my. s/reseults/results/ Looks like I'll be doing a v6 to add R-b and clean up these nits, as it is turning into too large a collection to ask a maintainer to do on my behalf. I'll try and split up some of the patches where I crammed multiple things into one commit, as part of the respin. >>>> >>>> Of particular note, we currently allow 'base' without 'discriminator= ' >>>> as a way to create a simple union with a base class. However, none >>>> of the existing QMP or QGA interfaces use it (it is exercised only >>>> in the testsuite). >>> >>> qapi-code-gen.txt documents 'base' only for flat unions. We should >>> either document its use in simple unions, or outlaw it. >> >> I went with outlaw later in the series, and the rest of my commit >> message was an attempt to explain my reasoning in that choice. But I >> can certainly try to improve the wording, if we need a respin. >=20 > If you respin, I suggest to add that it's undocumented. Sure. >> I'm hoping to add as a followup series a variant of simple unions that= >> is type-safe: >> >> [3] >> { 'union':'SimpleAndSafe', 'discriminator':'MyEnum', >> 'data':{ 'one':'str', 'two':'int' }} >> >> Hmm - that makes me wonder - do we support non-dict branches in a flat= >> union? The usual use case of a flat union is that all dictionary keys= >> in the branch's dictionary are treated as keys at the top level of the= >> resulting overall union; but a non-dictionary branch has no keys to >> flatten into the top level. >=20 > I think you just showed why non-dictionary branches make no sense. >=20 >> I may need to tweak a subsequent patch to= >> ensure that flat union branches can only use dictionaries (while simpl= e >> unions can use any type). >=20 > Yes, please. Follow-up patch okay, if that's easier for you. At this point, it may indeed be easier (keep the front half of the patch down to the bare minimum, and just make the entire series longer, rather than trying to rebase things into perfection). >>> What do you mean by "anonymous inline base type"? >> >> [5] basically, that the following example could be legal shorthand... >> >>> >>>> +{ 'enum': 'TestEnum', >>>> + 'data': [ 'value1', 'value2' ] } >>>> +{ 'type': 'TestTypeA', >>>> + 'data': { 'string': 'str' } } >>>> +{ 'type': 'TestTypeB', >>>> + 'data': { 'integer': 'int' } } >>>> +{ 'union': 'TestUnion', >>>> + 'base': { 'enum1': 'TestEnum', 'kind': 'str' }, >>>> + 'discriminator': 'TestEnum', >=20 > Shouldn't this be 'discriminator': 'enum1'? Yes. Good catch. >=20 > Since there's nothing currently broken, I wouldn't label the question > "should we allow an anonymous inline base type?" FIXME. As part of my respin, I'll change any FIXME into something more benign like TODO or RFC if it is merely intended to document a possible future direction and not and existing bug to be corrected by the series (so that the remaining additions of FIXME in the earlier part of the series will all disappear by the end of the series). >>> Since all my questions are about your intentions and rationale: >>> >>> Reviewed-by: Markus Armbruster >=20 > R-by stands, but I encourage you to polish the commit message and drop > the FIXME: from the comment in flat-union-bad-base.json. I think we've racked up enough to warrant a respin; I'll keep your R-b where the only things I touch are obviously trivial, but it may mean a few patches come through without R-b. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --1itXx1Q1VRC5Q37QdKXDulJvWkKjrdBAo Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJVFbO9AAoJEKeha0olJ0Nq/w4IAKIeXeSGK/iOdJiEEo5tjh/X mZJiV+g65ECtY/LKZ4MvI+NViXydgw+DsAG6au68DtotJGGl3zJXpnAfcUjuliH+ PMytlRfpFHuthFnvFAtIA3bv+0sE/dYvNh1oUPbAri8kvgGUgL5MJfCedof/AoXc DyutJ6SKi6yuUrNQxWnLgMOzW+8pjN5nqVOGFGseSNZNNNQnSd30/so5yPZCYwCw AeZNf3cteel1mLtOSCiBFhaIjcC/sUUkCNOfS2BqmSuPGFB0siQBisLaglyyAjDP YKc2ablNfvGAsiwDSd4Za7ken2GnwM1iZTqN3Oi+UgmDwEIo5NHNgW80Pt3+CiM= =Xr4z -----END PGP SIGNATURE----- --1itXx1Q1VRC5Q37QdKXDulJvWkKjrdBAo--