From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38551) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoeTw-00028g-PI for qemu-devel@nongnu.org; Tue, 20 Oct 2015 17:30:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZoeTr-0003Ng-Qi for qemu-devel@nongnu.org; Tue, 20 Oct 2015 17:30:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57298) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoeTr-0003NI-Id for qemu-devel@nongnu.org; Tue, 20 Oct 2015 17:30:03 -0400 References: <1444968943-11254-1-git-send-email-eblake@redhat.com> <1444968943-11254-4-git-send-email-eblake@redhat.com> <87y4eylqt7.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <5626B255.4060203@redhat.com> Date: Tue, 20 Oct 2015 15:29:57 -0600 MIME-Version: 1.0 In-Reply-To: <87y4eylqt7.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="XMsuBPnbWnMkcwA3kdWOdPClb1Sw3H1oc" Subject: Re: [Qemu-devel] [PATCH v9 03/17] qapi: Reserve 'u' and 'has[-_]*' member names 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) --XMsuBPnbWnMkcwA3kdWOdPClb1Sw3H1oc Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/19/2015 11:19 AM, Markus Armbruster wrote: > I'm not quite comfortable with reserving 'u' now, becaue I feel we > haven't fully explored the design space for avoiding branch - member > clashes. >=20 > I still like the basic idea to give the unnamed union a name. It needs= > to be a short one, to keep the C code legible. 'u' is an obvious > option, but it requires reserving 'u' at least as member name. '_u' > wouldn't. Alternatively, call the union 'u', but avoid the clash by > mapping QAPI member name 'u' to C identifier '_u'. Naming the union '_u' is a bit uglier (more typing in every client) whereas munging just the member name (what about 'q_u', the way c_name appends 'q_' to any other name that would otherwise collide) pushes the ugliness only to the C code that actually uses a member named 'u'. But the idea of teaching c_name() to munge 'u' as a member name certainly seems doable, at which point we no longer need to reserve 'u' as a member name. >=20 > I feel the decision should be made over the patch that give the union a= > name. Well, that's patch 7/17 of this series, so at most, all I need to do is shuffle things around when rebasing for v10. For that matter, if we make c_name() munge 'u', it can just as easily munge a member named 'has_' to 'q_has' (or 'base' to 'q_base' - except that we are getting rid of that); or whatever other names we burn for convenience on the C side. But no one is using a member named 'u' at the moment, so it's not the most critical problem to solve; and forbidding it is certainly conservative (we can relax things to allow the name in QMP after all, once we figure out the appropriate munging for the C side). >> +++ b/scripts/qapi.py >> @@ -488,6 +488,10 @@ def check_type(expr_info, source, value, allow_ar= ray=3DFalse, >> for (key, arg) in value.items(): >> check_name(expr_info, "Member of %s" % source, key, >> allow_optional=3Dallow_optional) >> + if key =3D=3D 'u' or key.startswith('has-') or key.startswith= ('has_'): >=20 > Something like c_name(key).startswith('has_') would avoid hardcoding th= e > mapping of '-' to '_' here. Dunno. Oh, nice idea. And looking at that, we have a number of places in qapi.py that are using things like str[-4:] =3D=3D '....' that might look nicer as str.endwith('....'). I may add an obvious trivial cleanup patch into the mix. >> @@ -588,6 +592,14 @@ def check_union(expr, expr_info): >> # Check every branch >> for (key, value) in members.items(): >> check_name(expr_info, "Member of union '%s'" % name, key) >> + # TODO: As long as branch names can collide with QMP names, w= e >> + # must prevent branches starting with 'has_'. However, we do = not >> + # need to reject 'u', because that is reserved for when we st= art >> + # sticking branch names in a C union named 'u'. >> + if key.startswith('has-') or key.startswith('has_'): >> + raise QAPIExprError(expr_info, >> + "Branch of union '%s' uses reserved n= ame '%s'" >> + % (name, key)) >=20 > This will go away again when we give the unnamed union a name. >=20 > I feel we should punt all further clash detection until late in the > cleanup work. It's merely nice to have (sane error message from > generator instead of possibly confusing one from the C compiler, > basically), and adding it now causes churn later on. Okay, I can respin along those lines - if my work later in the series removes a negative test added earlier in the series, then strip that test from the series as a whole rather than fighting the churn, to reduce the size of the series. >> +++ b/tests/qapi-schema/args-name-has.json >> @@ -1,6 +1,5 @@ >> # C member name collision >> -# FIXME - This parses, but fails to compile, because the C struct is = given >> -# two 'has_a' members, one from the flag for optional 'a', and the ot= her >> -# from member 'has-a'. Either reject this at parse time, or munge th= e C >> -# names to avoid the collision. >> +# This would attempt to create two 'has_a' members of the C struct, o= ne >> +# from the flag for optional 'a', and the other from member 'has-a'. >> +# TODO we could munge the optional flag name to avoid the collision. >=20 > You mean call them _has_FOO instead of has_FOO? The generated code > would be rather confusing... >=20 > If we don't want to reserve all names starting with 'has_', then I'd > narrowly outlaw having both an optional member FOO and a member has_FOO= =2E > I think I'd like that a bit better than outlawing 'has_'. But not > enough to accept much implementation complexity. The problem comes with child classes - we don't know a priori if an optional member in one struct will end up being a base class to another struct or union where the child class will hit the name clash. It's easier to outlaw the name, or else come up with a munging scheme that never clashes. Changing the existing has_ naming of flags is awkward (lots of existing code) compared to munging the (unlikely) addition of a new has_ member to a single qapi type. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --XMsuBPnbWnMkcwA3kdWOdPClb1Sw3H1oc 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/ iQEcBAEBCAAGBQJWJrJVAAoJEKeha0olJ0NqV9MH/2S21UQBMu6myIeiwpQpWw// BY/VK2W4Z4X19NZ+d1Z80rLZ3TN82CPRR+VAaMwWs0Dwa+rRXjWPijEOLQhnN9VZ t1HjTk/fSWsSNPgyV5w5GYGwYP/2V3qKC5NhZw3kUq9mzkDUlnoJ/I7JGBJOOCI+ 6h+Q3BymxOHSx/4Rj6qJ6xLWb07EgWUHcPM6v7+QcXaS1rzy1JfZZwBoKFhwpo5Y y8MZz3UNpBl5PRvYNQJQg07+rpTr5JC23Cs9eR0ivkAn4NnosXNDmhLLThhdEHOm 4OZ2EbuLAjOZZOWx58dh4lx4e1a1OYJEnewGTwa/caKG0kyuBxyfJDkWRg0+654= =mSLH -----END PGP SIGNATURE----- --XMsuBPnbWnMkcwA3kdWOdPClb1Sw3H1oc--