From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51986) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkYh3-0007XD-Vq for qemu-devel@nongnu.org; Fri, 09 Oct 2015 10:30:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZkYh0-00012G-L8 for qemu-devel@nongnu.org; Fri, 09 Oct 2015 10:30:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53859) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkYh0-00012B-Ak for qemu-devel@nongnu.org; Fri, 09 Oct 2015 10:30:42 -0400 References: <1443930073-19359-1-git-send-email-eblake@redhat.com> <1443930073-19359-10-git-send-email-eblake@redhat.com> <87pp0otc3y.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <5617CF8C.8060002@redhat.com> Date: Fri, 9 Oct 2015 08:30:36 -0600 MIME-Version: 1.0 In-Reply-To: <87pp0otc3y.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Kbx3IpesSFo0F5PS5C8HiEvpuPUiGlShR" Subject: Re: [Qemu-devel] [PATCH v7 09/14] qapi: Track owner of each object member List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Kbx3IpesSFo0F5PS5C8HiEvpuPUiGlShR Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/09/2015 07:17 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Future commits will migrate semantic checking away from parsing >> and over to the various QAPISchema*.check() methods. But to >> report an error message about an incorrect semantic use of a >> member of an object type, it helps to know which type, command, >> or event owns the member. Rather than making all the check() >> methods have to pass around additional information, it is easier >> to have each member track the name of the type that owns it in >> the first place. >=20 > Making members track their owner is easy enough (your patch is proof), > but asserting it's easier suggests you tried the other approach, too. > Did you? Yes, my first attempt was indeed to pass the QAPISchemaEntity being checked to each QAPISchemaObjectTypeMember.check() call... >=20 > In fact, passing the owner to the check() methods would be easy enough.= > QAPISchemaObjectType.check() passes self to its local members' .check()= =2E > Covers non-variant members. Variant members take a bit more effort, > because more classes (and thus more check() methods) are involved. > QAPISchemaObjectType.check() and QAPISchemaAlternateType.check() pass > self to QAPISchemaObjectTypeVariants.check(), which passes it on to > QAPISchemaObjectTypeVariant.check(), which passes it on to > QAPISchemaObjectTypeMember.check(). >=20 > I suspect the technique becomes cumbersome only when you start passing > members to helper functions: you have to pass the owner, too. If > members know their owner, passing a member suffices. =2E..One of the other advantages of my approach that you don't get when passing the owner to each check() call is that when it comes to base classes, the owner of a member inherited from a base class should be the base class, but the ObjectType that is calling the .check() method is instead the derived class. As soon as I realized that in my first approach, my next attempt was to pass more information through each element of the seen[] array, as in: seen[m.name] =3D {'member':m, 'owner':type} until I realized that it really was easier to just keep things with: seen[m.name] =3D m and have m.owner do the right thing. Having the member track the base class owner makes the error messages much nicer (as in "'name' (member of Sub) collides with 'name' (member of Base)" in a subsequent patch), which was not possible when the only thing being passed down was the current ObjectType being checked. >> +++ b/scripts/qapi.py >> @@ -961,8 +961,16 @@ class QAPISchemaObjectType(QAPISchemaType): >> assert base is None or isinstance(base, str) >> for m in local_members: >> assert isinstance(m, QAPISchemaObjectTypeMember) >> - assert (variants is None or >> - isinstance(variants, QAPISchemaObjectTypeVariants)) >> + assert not m.owner >> + m.owner =3D name >> + if variants is not None: >> + assert isinstance(variants, QAPISchemaObjectTypeVariants)= >> + if variants.tag_member: >> + assert not variants.tag_member.owner >> + variants.tag_member.owner =3D name >> + for v in variants.variants: >> + assert not v.owner >> + v.owner =3D name >=20 > Works, but rummaging in instances of other classes is not so nice. > Could instead do >=20 > for m in local_members: > m.set_owner(name) > if variants is not None: > variants.set_owner(name) >=20 > with the obvious set_owner() methods in QAPISchemaObjectTypeMember, > QAPISchemaObjectTypeVariants, QAPISchemaObjectTypeVariant. Yeah, I was debating about a set_owner() method - sounds like it is the way to go, and that I ought to post a v8 spin of this series. >> @@ -1034,6 +1044,15 @@ class QAPISchemaObjectTypeMember(object): >> def c_name(self): >> return c_name(self.name) >> >> + def describe(self): >> + source =3D self.owner >> + if source.startswith(':obj-'): >> + source =3D source[5:] >> + return "'%s' (%s of %s)" % (self.name, self._describe(), sour= ce) >> + >> + def _describe(self): >> + return 'member' >=20 > A simple class variable would do, wouldn't it? Are class variables polymorphic? (I guess I have to go figure out the python lookup rules for self.variable, to see if they differ from self.method()) >> @@ -1042,6 +1061,9 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObj= ectTypeMember): >> assert self.type.is_implicit(QAPISchemaEnumType) >> return 'kind' >> >> + def describe(self): >> + return "'kind' (implicit tag of %s)" % self.owner >> + >=20 > Let's compare to the inherited describe() with this one. >=20 > Why no need to strip a ':obj-' prefix here? Because no union type is implicit, so there is no ':obj-' prefix to strip= =2E >=20 > This prints "'kind' (implicit tag of ...)". The inherited describe() > would print "'type' (member of ...). >=20 > The 'kind' vs. 'type' difference doesn't matter, because neither name > occurs in the schema anyway. >=20 > "implicit tag of" is an improvement over "member of". >=20 > However, the inherited describe() already has a hook to replace the par= t > before the "of". Why do we need to override it wholesale anyway? The parent version prints self.name ('type'), but as long as we have the type/kind mismatch, we want to print the C name that is conflicting ('kind') and not self.name. But once I override describe() for that, it was easier to do a wholesale override. It all goes away later in the patch that renames kind to type. Maybe some comments here would help? >> @@ -1222,7 +1251,7 @@ class QAPISchema(object): >> def _make_implicit_object_type(self, name, info, role, members): >> if not members: >> return None >> - name =3D ':obj-%s-%s' % (name, role) >> + name =3D ':obj-%s %s' % (name, role) >> if not self.lookup_entity(name, QAPISchemaObjectType): >> self._def_entity(QAPISchemaObjectType(name, info, None, >> members, None)) >=20 > I know I suggested this, but I'm having second thoughts about spaces in= > name. The test output (visible below) becomes a bit confusing, and > unnecessarily hard to parse by ad hoc scripts (yes, I've done such > things from time to time). >=20 > We could keep >=20 > name =3D ':obj-%s-%s' % (name, role) >=20 > here, and replace the '-' by ' ' in describe(). Problematic if name or= > role can also contain '-'. Use a more suitable character to separate > the two then. Name can contain '-', role does not. Can always search for the last '-'. Or we could reorder things to put role first. Or use ':obj:%s:%s'. But yes, I agree that we can keep the implicit name space-free, and make describe() smarter at reverse-engineering it back into a human-readable description. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Kbx3IpesSFo0F5PS5C8HiEvpuPUiGlShR 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/ iQEcBAEBCAAGBQJWF8+MAAoJEKeha0olJ0NqB8kH/jGhdq0Wllfv1X0oGYWNJ5Rj VK/YNc9BaZ0ah6BcQF/FYPgsmxUtAZd4BaHwfwLugWLQbsdrvggFwxjzLbxtiJmD Zokmkd+UudFWiFdBJyPPVt/K2wfbWeVM2f2VLKR8IDU661ETM98tq37UZNKaDyCn /DnDWZ02+pZoZeK/E3+YjoXHXT3UjIiF7STXYD3UaCb5rnyWizhQdpITzxFZwn79 AYppzMDHqUivPu6vGVvc6ZtK55XYpAJODGTkb4YdcsGDz1fIADsI9BS3d1TZGTT5 wSit1qiuAe2G05/SKFvdiXGbuZ9TOM9uwZwDc8PWP2/sEh3tYkMMgx2bdhg+yko= =ivlt -----END PGP SIGNATURE----- --Kbx3IpesSFo0F5PS5C8HiEvpuPUiGlShR--