From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37845) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yb7gT-0005qA-5i for qemu-devel@nongnu.org; Thu, 26 Mar 2015 09:18:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yb7gO-0005qo-Og for qemu-devel@nongnu.org; Thu, 26 Mar 2015 09:18:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51005) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yb7gO-0005qW-Ge for qemu-devel@nongnu.org; Thu, 26 Mar 2015 09:18:48 -0400 From: Markus Armbruster References: <1427227433-5030-1-git-send-email-eblake@redhat.com> <1427227433-5030-7-git-send-email-eblake@redhat.com> Date: Thu, 26 Mar 2015 14:18:43 +0100 In-Reply-To: <1427227433-5030-7-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 24 Mar 2015 14:03:31 -0600") Message-ID: <87h9t76gng.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Eric Blake Cc: kwolf@redhat.com, lcapitulino@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, wenchaoqemu@gmail.com 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. > > 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. > Meanwhile, it would be nice to allow > 'discriminator':'EnumType' without 'base' for creating a simple union > that is type-safe rather than open-coded to a generated enum (right > now, we are only type-safe when using a flat union, but that uses > a different syntax of 'discriminator':'member-name' which requires > a base class containing a 'member-name' enum field). I'm afraid I don't get you. Can you give examples? > If both 'base' > and 'discriminator' are optional, then converting a simple union > with base class to a type-safe simple union with an enum discriminator > would not be possible. So my plan is to get rid of 'base' without > 'discriminator' later in the series; Aha: you're going to outlaw 'base' in simple unions. Yes, please. > this will be no real loss, as any > union that needs additional common fields can always use a flat > union. The mathematical concept behind unions is the sum type. We have three implementations, and we call them simple, flat, anonymous. Anonymous unions are implicitly tagged. They come with the obvious restrictions required to make implicit tagging work. The other two are explicitly tagged. The difference between them is just notation. I like my unions flat, because for me the extra wrapping is just notational overhead. In particular, I can define simple unions in terms of flat ones by restricting all union cases to a single member named 'data'. They're not implemented that way, but that's a historical accident. Simple unions are a redundant concept. This proves your "can always use a flat union" proposition. The only way they can earn their keep is making the schema easier to read. I haven't thought about that. Another historical accident is how we express members common to all union cases: base type. Probably just because complex types already had them. Are you planning to change anything there? [...] > diff --git a/tests/qapi-schema/alternate-nested.err b/tests/qapi-schema/alternate-nested.err > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/alternate-nested.exit b/tests/qapi-schema/alternate-nested.exit > new file mode 100644 > index 0000000..573541a > --- /dev/null > +++ b/tests/qapi-schema/alternate-nested.exit > @@ -0,0 +1 @@ > +0 > diff --git a/tests/qapi-schema/alternate-nested.json b/tests/qapi-schema/alternate-nested.json > new file mode 100644 > index 0000000..d5812bf > --- /dev/null > +++ b/tests/qapi-schema/alternate-nested.json > @@ -0,0 +1,7 @@ > +# FIXME: we should reject a nested anonymous union branch Same reason we want to reject nested / anonymous complex types elsewhere? Or is there a deeper reason? > +{ 'union': 'Union1', > + 'discriminator': {}, > + 'data': { 'name': 'str', 'value': 'int' } } > +{ 'union': 'Union2', > + 'discriminator': {}, > + 'data': { 'nested': 'Union1' } } > diff --git a/tests/qapi-schema/alternate-nested.out b/tests/qapi-schema/alternate-nested.out > new file mode 100644 > index 0000000..0137c1f > --- /dev/null > +++ b/tests/qapi-schema/alternate-nested.out > @@ -0,0 +1,5 @@ > +[OrderedDict([('union', 'Union1'), ('discriminator', OrderedDict()), ('data', OrderedDict([('name', 'str'), ('value', 'int')]))]), > + OrderedDict([('union', 'Union2'), ('discriminator', OrderedDict()), ('data', OrderedDict([('nested', 'Union1')]))])] > +[{'enum_name': 'Union1Kind', 'enum_values': None}, > + {'enum_name': 'Union2Kind', 'enum_values': None}] > +[] [...] > diff --git a/tests/qapi-schema/flat-union-bad-base.err b/tests/qapi-schema/flat-union-bad-base.err > new file mode 100644 > index 0000000..5962ff4 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-bad-base.err > @@ -0,0 +1 @@ > +tests/qapi-schema/flat-union-bad-base.json:9: Base 'OrderedDict([('enum1', 'TestEnum'), ('kind', 'str')])' is not a valid type > diff --git a/tests/qapi-schema/flat-union-bad-base.exit b/tests/qapi-schema/flat-union-bad-base.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/flat-union-bad-base.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/flat-union-bad-base.json b/tests/qapi-schema/flat-union-bad-base.json > new file mode 100644 > index 0000000..d69168f > --- /dev/null > +++ b/tests/qapi-schema/flat-union-bad-base.json > @@ -0,0 +1,13 @@ > +# we require the base to be an existing complex type > +# FIXME: should we allow an anonymous inline base type? What do you mean by "anonymous inline base type"? > +{ 'enum': 'TestEnum', > + 'data': [ 'value1', 'value2' ] } > +{ 'type': 'TestTypeA', > + 'data': { 'string': 'str' } } > +{ 'type': 'TestTypeB', > + 'data': { 'integer': 'int' } } > +{ 'union': 'TestUnion', > + 'base': { 'enum1': 'TestEnum', 'kind': 'str' }, > + 'discriminator': 'TestEnum', > + 'data': { 'kind1': 'TestTypeA', > + 'kind2': 'TestTypeB' } } > diff --git a/tests/qapi-schema/flat-union-bad-base.out b/tests/qapi-schema/flat-union-bad-base.out > new file mode 100644 > index 0000000..e69de29 [...] "Doesn't deal well" is an understatement :) Since all my questions are about your intentions and rationale: Reviewed-by: Markus Armbruster