From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35489) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzAvJ-0001th-4t for qemu-devel@nongnu.org; Wed, 18 Nov 2015 17:09:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZzAvF-0003wA-QI for qemu-devel@nongnu.org; Wed, 18 Nov 2015 17:09:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46660) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzAvF-0003vx-Hj for qemu-devel@nongnu.org; Wed, 18 Nov 2015 17:09:49 -0500 References: <1447836791-369-1-git-send-email-eblake@redhat.com> <1447836791-369-28-git-send-email-eblake@redhat.com> <87poz7me0g.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <564CF727.8020507@redhat.com> Date: Wed, 18 Nov 2015 15:09:43 -0700 MIME-Version: 1.0 In-Reply-To: <87poz7me0g.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="4KLOVS4KvEDWov1axeSM1LscE8VU0G4DM" Subject: Re: [Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes 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) --4KLOVS4KvEDWov1axeSM1LscE8VU0G4DM Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/18/2015 10:08 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> In general, designing user interfaces that rely on case >> distinction is poor practice. Another benefit of enforcing >> a restriction against case-insensitive clashes is that we >> no longer have to worry about the situation of enum values >> that could be distinguished by case if mapped by c_name(), >> but which cannot be distinguished when mapped to C as >> ALL_CAPS by c_enum_const() [via c_name(name, False).upper()]. >> Thus, having the generator look for case collisions up front >> will prevent developers from worrying whether different >> munging rules for member names compared to enum values as a >> discriminator will cause any problems in qapi unions. >> >> @@ -378,9 +379,9 @@ def check_name(expr_info, source, name, allow_opti= onal=3DFalse, >> # code always prefixes it with the enum name >> if enum_member and membername[0].isdigit(): >> membername =3D 'D' + membername >> - # Reserve the entire 'q_' namespace for c_name() >> + # Reserve the entire 'q_'/'Q_' namespace for c_name() >> if not valid_name.match(membername) or \ >> - c_name(membername, False).startswith('q_'): >> + c_name(membername, False).upper().startswith('Q_'): >> raise QAPIExprError(expr_info, >> "%s uses invalid name '%s'" % (source, na= me)) I'll switch to lower() here, for consistency. > Now let me try claw back some clarity. >=20 > First try: refine your approach. >=20 > Observe that all but one caller of lookup_entity() actually look up > types. The one caller that doesn't is _def_entity(). Change it to: >=20 > cname =3D c_name(ent.name) > if isinstance(ent, QAPISchemaEvent): > cname =3D cname.upper() > else: > cname =3D cname.lower() > if self._lookup_entity(cname) I think lookup_entity would need two parameters: the munged name, and the actual name... > raise QAPIExprError(ent.info, > "Entity '%s' already defined" % end.nam= e) > if cname in self._entity_dict: > raise QAPIExprError(ent.info, > "Entity '%s' collides with '%s'" > % (ent.name, self._entity_dict[cname].n= ame)) >=20 > where _lookup_entity() is back to the much simpler version: >=20 > ent =3D self._entity_dict.get(cname) > if ent and ent.name !=3D cname =2E..because ent.name does not have to match cname. > ent =3D None >=20 > lookup_type() becomes: >=20 > def lookup_type(self, name, typ=3DQAPISchemaType): > return self._lookup_entity(name, typ) I don't know that lookup_type needs an optional parameter. >=20 > and the third caller _make_implicit_object_type() goes from >=20 > if not self.lookup_entity(name, QAPISchemaObjectType): >=20 > to >=20 > if not self.lookup_type(name, QAPISchemaObjectType): And a plain lookup_type(name) is what we should have been using here all along. >=20 > Another possible improvement is hiding the case-folding logic in > methods. Have a QAPISchemaEntity.c_namecase() that returns > self.c_name().lower(). Overwrite it in QAPISchemaEvent to return > .upper() instead. Use it to make _def_entity() less ugly. >=20 > Probably not worthwhile unless more uses of .c_namecase() pop up. I kind of like it. I think I'll propose a 26.5 that implements that, then redo 27 on top of it (without reposting the entire series). >=20 > Second approach: don't use _entity_dict for clash detection! Have a > second dictionary. I debated about doing that the first time around. But it's probably a bit easier to follow, so I think I'll do it. >> @@ -1390,7 +1414,8 @@ class QAPISchema(object): >> >> def visit(self, visitor): >> visitor.visit_begin(self) >> - for (name, entity) in sorted(self._entity_dict.items()): >> + for entity in sorted(self._entity_dict.values(), >> + key=3Dattrgetter('name')): >> if visitor.visit_needed(entity): >> entity.visit(visitor) >> visitor.visit_end() >=20 > Cool trick. and with two dicts, I wouldn't need this trick. >> +++ b/tests/qapi-schema/command-type-case-clash.err >> @@ -0,0 +1 @@ >> +tests/qapi-schema/command-type-case-clash.json:3: Entity 'foo' collid= es with 'Foo' >=20 > The message's location is foo's. An additional message "'Foo' defined > here" would be nice. Just an idea, could be done later. The way we currently generate a single line message is by raising a python exception. I guess we could rework things to build up lines at a time, then finally raise the complete multi-line message on a pre-formed string, rather than the current building the message as a parameter to the exception constructor. But any changes on this front will have to wait for later. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --4KLOVS4KvEDWov1axeSM1LscE8VU0G4DM 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/ iQEcBAEBCAAGBQJWTPcnAAoJEKeha0olJ0NqAsMH/3L1eZ/Xrddqicx3glzTAeyW PzqZkDutEnDLAvZLuKqhvDp7joxAGRSk0Q63vUoyCUVRVb9OIi1thVdfkvOOAP53 gTjMeJZQIS/5GgjFrft6KG0rNr2231mDpIWe3MT9YSmnUV6QSCpnI3zFAvOHWEaY QSj2/XqBC5CK0c7T5foltAq1o7ejWFyDxr5dkchupBbHTRYQhb+dWWjeLy+xYTgd xSt3YBBP9s+E/LkoHQK0HpUCpWCzvzobtcnXdQ0WGMkuzA9qNDD+FcmooFmDhoci rPzaT1WSBaMLa+X61LHPDz1a84E7rGdEq1vMzveGqLQ+/jpJzJDO3Qbjn1ji+u0= =9ozo -----END PGP SIGNATURE----- --4KLOVS4KvEDWov1axeSM1LscE8VU0G4DM--