From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47008) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwuQd-0006y0-Rt for qemu-devel@nongnu.org; Thu, 12 Nov 2015 11:08:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwuQZ-0007pP-B9 for qemu-devel@nongnu.org; Thu, 12 Nov 2015 11:08:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37805) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwuQZ-0007pL-3S for qemu-devel@nongnu.org; Thu, 12 Nov 2015 11:08:47 -0500 References: <1447224690-9743-1-git-send-email-eblake@redhat.com> <1447224690-9743-28-git-send-email-eblake@redhat.com> <87oaezi5ks.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <5644B987.5080506@redhat.com> Date: Thu, 12 Nov 2015 09:08:39 -0700 MIME-Version: 1.0 In-Reply-To: <87oaezi5ks.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JDDlKE2XrnoafiMiK7cO16E2L4NiIUi10" Subject: Re: [Qemu-devel] [PATCH v11 27/28] 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) --JDDlKE2XrnoafiMiK7cO16E2L4NiIUi10 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/12/2015 08:46 AM, 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 >> mapping to the same enum value, even when the names do not >> collide case-wise. So for a decent error message, we have to >> determine if the enum is implicit (and if so where the real >> collision lies). >> >> @@ -873,8 +856,30 @@ class QAPISchemaEnumType(QAPISchemaType): >> self.values =3D values >> self.prefix =3D prefix >> >> + def _describe(self, schema): >> + # 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): >> + return "Alternate '%s' branch" % owner.name >=20 > Didn't we just drop this kind of implicit enum? D'oh; rebase churn reordering patches. >=20 >> + else: >> + return "Union '%s' branch" % owner.name >> + else: >> + return "Enum '%s' value" % self.name >=20 > I like to call it "member" rather than value, because it avoids > confusion with the numeric value of the C enumeration constant generate= d > for it. Sure, that sounds slightly better. >=20 > The conditional isn't exactly elegant, but it would do. I'm not 100% > convinced we need it, though. self.info already points to whatever > defined this enum, either an explicit enum definition, or a simple unio= n > definition. How do the error messages come out if we dumb down to > "Member '%s'"? >=20 > A method with a similar purpose exists in QAPISchemaObjectTypeMember, > but it's spelled describe(). It's used only from within the class. Are you talking about _pretty_owner()? Or the actual describe() that calls _pretty_owner()? > Rename it to match this one? >=20 >> + >> 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: >> + raise QAPIExprError(self.info, >> + "%s '%s' clashes with '%s'" >> + % (self._describe(schema), value,= >> + seen[c_value])) >> + seen[c_value] =3D value >=20 > Loop body is very similar to QAPISchemaObjectTypeMember.check_clash(). > Differences: >=20 > * c_enum_const(enum_name, member_name) vs. c_name(member_name).upper() >=20 > This isn't really a difference, because the former returns the latter= > prefixed by a string that doesn't vary in the loop. Well, it _is_ a difference if c_name() munges differently than c_enum_const() (the whole question of whether 'OneTwo' and 'One-Two' are detected as clashes); but then again, I have the patches that try to rework c_enum_const() to use just c_name().upper(). >=20 > One could argue that using c_enum_const() lets us abstract from what > it does, but that's an illusion. We rely on it when we generate unio= n > members called c_name(member_name) without checking for collisions > again. >=20 > By the way, c_enum_const(self.name, value, self.prefix) would be more= > correct. Doesn't matter here, of course. >=20 > Therefore, I'd be very much tempted to use c_name(member_name).upper(= ) > here as well. >=20 > * The error message. But I suspect the same "Member '%s' clashes with > '%s'" could do for both. >=20 > If I'm right and we can drop the differences, the common code could > become a helper function. I'll play with it and see what pops out. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --JDDlKE2XrnoafiMiK7cO16E2L4NiIUi10 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/ iQEcBAEBCAAGBQJWRLmHAAoJEKeha0olJ0Nq6KEH/j1esQnOtHBfw2uhsZhJArvo qi1myWyjujU3GW3SCyigao1bR+B34ok+n/72hsze9COIxlgJBpCZqLQSfOFHL66l MMhtAC4/n7FNB6KzstZ2A7JvhADMqnSO4fPU6qvbe/sEaX0OkyANUBHrtjrIYEc0 rxBqJPHdaNjBFPx0/dntimmQfsOuVD7g+1eLzotiRuTN1sr6ELetvkekjbyk5isA k8kbyuTQq6UhQHU7hl9W5F0/x0xK/k/FloAMwLviyU3XEc1DBgb9Sr5m54r8Kg9m VGkQeEyxOV9+MbIAR6VJxcwaWQT8RbGQQ2kThU7QBsEdNX/QufH21WddD+82CS8= =Fkq9 -----END PGP SIGNATURE----- --JDDlKE2XrnoafiMiK7cO16E2L4NiIUi10--