From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50741) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmOXd-0002hh-Ti for qemu-devel@nongnu.org; Wed, 14 Oct 2015 12:04:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZmOXZ-0006O9-Oy for qemu-devel@nongnu.org; Wed, 14 Oct 2015 12:04:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56959) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmOXZ-0006Ng-HO for qemu-devel@nongnu.org; Wed, 14 Oct 2015 12:04:33 -0400 From: Markus Armbruster References: <1443930073-19359-8-git-send-email-eblake@redhat.com> <1444307217-16306-1-git-send-email-armbru@redhat.com> <561E55A2.6000401@redhat.com> Date: Wed, 14 Oct 2015 18:04:30 +0200 In-Reply-To: <561E55A2.6000401@redhat.com> (Eric Blake's message of "Wed, 14 Oct 2015 07:16:18 -0600") Message-ID: <87zizlv3ld.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [RFC PATCH] qapi: Rename simple union's generated tag member to type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, mdroth@linux.vnet.ibm.com Eric Blake writes: > On 10/08/2015 06:26 AM, Markus Armbruster wrote: >> Struct and union type members are generally named alike in QMP and C, >> except for a simple union's implicit tag member, which is "type" in >> QMP, and "kind" in C. Can't change QMP, so rename it in C. >> >> Since the implicit enumeration type is still called *Kind, this >> doesn't clean up the type vs. kind mess completely. >> >> Signed-off-by: Markus Armbruster >> --- > ... >> scripts/qapi-types.py | 12 ++++-------- >> scripts/qapi-visit.py | 15 ++++----------- >> tests/test-qmp-commands.c | 2 +- >> tests/test-qmp-input-visitor.c | 30 +++++++++++++++--------------- >> tests/test-qmp-output-visitor.c | 8 ++++---- >> tpm.c | 2 +- >> ui/input-keymap.c | 10 +++++----- >> ui/input-legacy.c | 2 +- >> ui/input.c | 22 +++++++++++----------- >> util/qemu-sockets.c | 12 ++++++------ >> 33 files changed, 113 insertions(+), 123 deletions(-) > > This touches a lot of files all in one commit (both your RFC and my v5 > version), and then I get to touch the same files all over again when I > swap to a named rather than anonymous union in the C struct. So here's > what I'm currently playing with: > > first patch: hack _just_ scripts/qapi*py to turn: > > { 'union':'Foo', 'data':{'a':'int','b':'bool'}} > > into: > > struct Foo { > union { > FooKind type; > FooKind kind; /* temporary hack */ > }; > union { /* union tag is @type */ > void *data; /* will go away much later in series */ > int64_t a; > bool b; > union { > void *data; > int64_t a; > int b; > } u; > }; > }; Gross :) I have to admit the idea occurred to me, too. > so that old code accessing foo->kind and foo->a just works, but also > leaving the door open to access foo->type and foo->u.a. Then a series > of patches grouped by logical file sets (so no one patch is too huge to > review, and spreading the load among maintainers), and a final patch to > scripts/qapi*.py to get to the desired: > > struct Foo { > FooKind type; > union { /* union tag is @type */ > void *data; /* will go away much later in series */ > int64_t a; > bool b; > } u; > }; The patches for renaming kind to type and for splicing in u. are mechanical. If we want to split them up, we need your hack. We may want to split along maintenance boundaries and route the parts through the respective maintainers. Slooow. We may want to split just for easier review, but still route the whole set through my tree in one go. I'm not sure splitting is necessary, however. > at which point we've gotten rid of any collisions between tag value > (branch names) and QMP names, at the possible expense of a new collision > with 'u'. In other words, we trade C-only collisions between two sets of user-defined names (branch names / tag values and member names) for reserving one more fixed name. Makes sense to me. > I'm also beefing up the testsuite and check_name() function > (or maybe somewhere else, still haven't actually written that part of my > planned series) to reject 'u' and anything spelled 'has_*' as member > names, to proactively avoid the need to worry about collisions with 'u' > or the added members for optional fields. I'll probably also reject > '*List' as a user-supplied type name, addressing the comment just barely > added in current qapi-next that we don't have a reserved namespace for > arrays. I think we should consider delaying the work on detecting C collisions in qapi.py, for a couple of reasons: * Any collisions qapi.py misses will be detected by the C compiler, thus qapi.py missing collisions is just an annoyance. * Our current plans involve removing some collisions (like branch name with member name), and add others (like member name with 'u'). * Doing collision detection late avoids churn, and can hopefully work with simpler and more regular code. If you want to add more tests flagging more of the current troublemakers, that's okay. I'd focus on other things, though. When we get to collision detection, we'll have to analyze them again anyway. > The collision with 'data' is harder; I can't remove it until we delete > visit_start_union()/visit_end_union() which starts to get in the mess of > patches that work with qapi visitor interfaces, so that will be in a > later subset. Delaying removal of 'data' until later feels just fine. > It may be a day or two before I can post the pending work. Meanwhile, I > previously posted subset C which is somewhat orthogonal (not sure if it > needs any minor rebasing to apply against current qapi-next), if you > want to dive into reviewing that, instead of waiting: > https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01980.html If I can find time for QAPI before you post again, I'll know where to find subset C :)