Re: [Qemu-devel] [RFC PATCH] qapi: Rename simple union's generated tag member to type

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>> ---
> ...
>>  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 :)