Eric Blake <ebl...@redhat.com> writes: > On 10/22/2015 08:01 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> writes: >> >>> We have two issues with our qapi union layout: >>> 1) Even though the QMP wire format spells the tag 'type', the >>> C code spells it 'kind', requiring some hacks in the generator. >>> 2) The C struct uses an anonymous union, which places all tag >>> values in the same namespace as all non-variant members. This >>> leads to spurious collisions if a tag value matches a QMP name. >>> >>> Make the conversion to the new layout for testsuite code. >>> >>> Note that this includes updating an error message regarding a >>> collision. After the conversion to the new union qapi layout >>> is complete, there will still be further patches for cleaning >>> up collision detection, since the use of a named union can >>> completely eliminate the collision wording changed here. >>> >>> Signed-off-by: Eric Blake <ebl...@redhat.com> >>> >>> --- >>> v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC: >>> http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html >>> --- >>> scripts/qapi.py | 2 +- >>> tests/qapi-schema/union-clash-type.err | 2 +- >>> tests/qapi-schema/union-clash-type.json | 6 +-- >>> tests/test-qmp-commands.c | 4 +- >>> tests/test-qmp-input-visitor.c | 78 >>> ++++++++++++++++----------------- >>> tests/test-qmp-output-visitor.c | 42 +++++++++--------- >>> 6 files changed, 66 insertions(+), 68 deletions(-) >>> >>> diff --git a/scripts/qapi.py b/scripts/qapi.py >>> index 1e7e08b..aab2b46 100644 >>> --- a/scripts/qapi.py >>> +++ b/scripts/qapi.py >>> @@ -546,7 +546,7 @@ def check_union(expr, expr_info): >>> base = expr.get('base') >>> discriminator = expr.get('discriminator') >>> members = expr['data'] >>> - values = {'MAX': '(automatic)', 'KIND': '(automatic)'} >>> + values = {'MAX': '(automatic)', 'TYPE': '(automatic tag)'} >>> >>> # Two types of unions, determined by discriminator. >>> >> >> Umm, does this really belong to this patch? > > Yes, because it cleans up the error message in union-clash-type.err, as > mentioned in the commit message. But since I'm already splitting out the > qapi-visit parts of 7/17, maybe it belongs better in that patch (all > changes to the rest of qapi to deal with the qapi-type parts)?
Makes sense, I think. >>> diff --git a/tests/qapi-schema/union-clash-type.err >>> b/tests/qapi-schema/union-clash-type.err >>> index a5dead1..c14bbdd 100644 >>> --- a/tests/qapi-schema/union-clash-type.err >>> +++ b/tests/qapi-schema/union-clash-type.err >>> @@ -1 +1 @@ >>> -tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion' >>> member 'kind' clashes with '(automatic)' >>> +tests/qapi-schema/union-clash-type.json:6: Union 'TestUnion' >>> member 'type' clashes with '(automatic tag)' > > At any rate, this is what improved by adjusting that line of qapi.py. > >>> diff --git a/tests/qapi-schema/union-clash-type.json >>> b/tests/qapi-schema/union-clash-type.json >>> index cfc256b..641b2d5 100644 >>> --- a/tests/qapi-schema/union-clash-type.json >>> +++ b/tests/qapi-schema/union-clash-type.json >>> @@ -1,9 +1,7 @@ >>> # Union branch 'type' >>> # Reject this, because we would have a clash in generated C, between the >>> -# simple union's implicit tag member 'kind' and the branch name 'kind' >>> +# simple union's implicit tag member 'type' and the branch name 'type' >>> # within the union. >>> -# TODO: Even when the generated C is switched to use 'type' rather than >>> -# 'kind', to match the QMP spelling, the collision should still be >>> detected. >>> -# Or, we could munge the branch name to allow compilation. >>> +# TODO: If desired, we could munge the branch name to allow compilation. >> >> Let's mark it TODO only if we intend to revisit the idea of munging >> branch names. > > I have a later patch queued that deletes this test altogether, so for > v10 I'll probably just eliminate changes here for less churn. > https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04111.html > >> >>> { 'union': 'TestUnion', >>> 'data': { 'kind': 'int', 'type': 'str' } } >> [Mind-numbing mechanical switch to u. and from kind to type...] >> > > Yep. I wish I knew coccinelle well enough to see if it could do the > conversion for me, but I ended up doing it by hand (basically by > applying 16/17 early, then seeing what failed to compile, fixing it up, > then rebasing it into position). I might be able to sledgehammer it into service here, but since you've done the job already...