From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm5Og-0001Jc-7u for qemu-devel@nongnu.org; Tue, 13 Oct 2015 15:38:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zm5Oc-0000hl-Rv for qemu-devel@nongnu.org; Tue, 13 Oct 2015 15:38:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39298) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm5Oc-0000hJ-KJ for qemu-devel@nongnu.org; Tue, 13 Oct 2015 15:38:02 -0400 References: <1444710158-8723-1-git-send-email-eblake@redhat.com> <1444710158-8723-17-git-send-email-eblake@redhat.com> <87r3kymxaj.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <561D5D94.9050307@redhat.com> Date: Tue, 13 Oct 2015 13:37:56 -0600 MIME-Version: 1.0 In-Reply-To: <87r3kymxaj.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="f04DhdpGQPmmtjAOeWAAH1kJDBnKiKScx" Subject: Re: [Qemu-devel] [PATCH v8 16/18] qapi: Move duplicate enum value checks to schema check() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --f04DhdpGQPmmtjAOeWAAH1kJDBnKiKScx Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/13/2015 12:35 PM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Similar to the previous commit, move the detection of a collision >> in enum values from parse time to QAPISchemaEnumType.check(). >> This happens to also detect collisions in union branch names, >> so for a decent error message, we have to determine if the enum >> is implicit (and if so where the real collision lies). >> >> Testing this showed that the test union-bad-branch and >> union-clash-branches were basically testing the same thing; >> with the minor difference that the former clashes only in the >> enum, while the latter also clashes in the C union member >> names that would be generated. So delete the weaker test. >> >> No change to generated code. >> >> Signed-off-by: Eric Blake >> >> +++ b/scripts/qapi.py >> @@ -527,7 +527,6 @@ def check_union(expr, expr_info): >> base =3D expr.get('base') >> discriminator =3D expr.get('discriminator') >> members =3D expr['data'] >> - values =3D {'MAX': '(automatic)', 'KIND': '(automatic)'} >=20 > Stupid / tired question: I can see 'MAX' in the new code further down, > but I can't see 'KIND'. Why? Was it perhaps covered by the previous > patch? MAX is covered by enum collisions. 'kind' is covered by the previous patch's non-variant vs. branch-name collisions. Hmm, maybe when respinning this I can simplify 15/18 to drop KIND from this spot (because KIND is not part of the enum). >> >> def check(self, schema): >> - assert len(set(self.values)) =3D=3D len(self.values) >> + # Check for collisions on the generated C enum values >> + seen =3D {c_enum_const(self.name, 'MAX'): '(automatic MAX)'} >> + for value in self.values: >> + c_value =3D c_enum_const(self.name, value) >> + if c_value in seen: >> + # If the enum is implicit, report the error on behalf= of >> + # the union or alternate that triggered the enum >> + if self.is_implicit(): >> + owner =3D schema.lookup_type(self.name[:-4]) >> + assert owner >> + if isinstance(owner, QAPISchemaAlternateType): >> + description =3D "Alternate '%s' branch" % own= er.name >> + else: >> + description =3D "Union '%s' branch" % owner.n= ame >> + else: >> + description =3D "Enum '%s' value" % self.name >=20 > Computing a reasonable description distracts from the checking job. > Suggest to outline this into a private method. Good idea. >> +++ b/tests/qapi-schema/alternate-clash.err >> @@ -1 +1 @@ >> -tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_= b' clashes with 'a-b' >> +tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch 'a_= b' clashes with 'a-b' >=20 > Our terminology isn't consistent: we use both "branch" and "case". Not= > this patch's problem to fix. >=20 >> diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-sche= ma/enum-clash-member.err >> index 48bd136..84030c5 100644 >> --- a/tests/qapi-schema/enum-clash-member.err >> +++ b/tests/qapi-schema/enum-clash-member.err >> @@ -1 +1 @@ >> -tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE= ' clashes with 'one' >> +tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' value 'ONE'= clashes with 'one' >=20 > I actually prefer calling ONE a member of MyEnum, because that leaves > value for its actual value. >=20 > For what it's worth, the C standard also talks about "members of an > enumeration". Easy enough to fix. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --f04DhdpGQPmmtjAOeWAAH1kJDBnKiKScx 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/ iQEcBAEBCAAGBQJWHV2UAAoJEKeha0olJ0NqwCwH/0MBejFomIL37fIgxco07/LG 0XnQl061ceBYWF5+fmyEBCSZ7wiCK6ACoKBSmjlutWAlZTRMVYKZuSjCmJ1SPZt5 eXHBhjlc0T9FXQ0w0GdZK+5eZhr3nLZuYii7xEOamMT23c1KestC5xZYm8d49YkO ElEKbF61YdDUf4jCGOBgCALalheJ36LKsY92ABpawjGjSsCXhZwWbuM1EIxtcXGx NuPAfhMNvbmogK9zRiVvqphroA37Drj3MjRrJPYWey2qvZRuMnLannSbHPC0kKDc UNUipwHOj1KglC71U7YdN9QVqhQ4njNRui88tZfzw+cPOXKZnYsGjhkgK2lV/GY= =2h0+ -----END PGP SIGNATURE----- --f04DhdpGQPmmtjAOeWAAH1kJDBnKiKScx--