| Message ID | 1456443528-13901-13-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > The generator special-cased > > { 'command':'foo', 'data': {} } > > to avoid emitting a visitor variable, but failed to see that > > { 'struct':'NamedEmptyType, 'data': {} } > { 'command':'foo', 'data':'NamedEmptyType' } > > needs the same treatment. There, the generator happily generates a > visitor to get no arguments, and a visitor to destroy no arguments; > and the compiler isn't happy with that, as demonstrated by the updated > qapi-schema-test.json: > > tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’: > tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used [-Werror=unused-but-set-variable] > Visitor *v; > ^ > > No change to generated code except for the testsuite addition. Using a named empty type when you could just as well use {} is a rather remote corner case, and I therefore don't care how we translate it as long as it works. We could make the generator suppress just the variable, or suppress the warning, or suppress the whole "get the arguments" thing. Your patch shows that the latter is easy enough, so why not. > Signed-off-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > v2: no change > v1: enhance commit message > Previously posted as part of qapi cleanup subset E: > v9: no change > v8: no change > v7: no change > v6: new patch > --- > scripts/qapi-commands.py | 6 +++--- > tests/test-qmp-commands.c | 5 +++++ > tests/qapi-schema/qapi-schema-test.json | 2 ++ > tests/qapi-schema/qapi-schema-test.out | 2 ++ > 4 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index f44e01f..7c9cfbf 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -65,7 +65,7 @@ def gen_marshal_vars(arg_type, ret_type): > ''', > c_type=ret_type.c_type()) > > - if arg_type: > + if arg_type and not arg_type.is_empty(): Since variants aren't possible here, arg_type and arg_type.members would accomplish the same, wouldn't it? Kills the idea to use the bug fix as motivation for .is_empty()... > ret += mcgen(''' > QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); > QapiDeallocVisitor *qdv; > @@ -97,7 +97,7 @@ def gen_marshal_vars(arg_type, ret_type): > def gen_marshal_input_visit(arg_type, dealloc=False): > ret = '' > > - if not arg_type: > + if not arg_type or arg_type.is_empty(): Likweise. > return ret > > if dealloc: > @@ -177,7 +177,7 @@ def gen_marshal(name, arg_type, ret_type): > > # 'goto out' produced by gen_marshal_input_visit->gen_visit_members() > # for each arg_type member, and by gen_call() for ret_type > - if (arg_type and arg_type.members) or ret_type: > + if (arg_type and not arg_type.is_empty()) or ret_type: This is just for consistency with the two previous hunks. > ret += mcgen(''' > > out: > diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c > index d6171f2..650ba46 100644 > --- a/tests/test-qmp-commands.c > +++ b/tests/test-qmp-commands.c > @@ -13,6 +13,11 @@ void qmp_user_def_cmd(Error **errp) > { > } > > +Empty2 *qmp_user_def_cmd0(Error **errp) > +{ > + return g_new0(Empty2, 1); > +} > + > void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp) > { > } > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json > index b5d0c53..33e8517 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -18,6 +18,8 @@ > { 'struct': 'Empty1', 'data': { } } > { 'struct': 'Empty2', 'base': 'Empty1', 'data': { } } > > +{ 'command': 'user_def_cmd0', 'data': 'Empty2', 'returns': 'Empty2' } > + > # for testing override of default naming heuristic > { 'enum': 'QEnumTwo', > 'prefix': 'QENUM_TWO', > diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out > index 225e2db..6b97fa5 100644 > --- a/tests/qapi-schema/qapi-schema-test.out > +++ b/tests/qapi-schema/qapi-schema-test.out > @@ -203,6 +203,8 @@ command guest-sync :obj-guest-sync-arg -> any > gen=True success_response=True > command user_def_cmd None -> None > gen=True success_response=True > +command user_def_cmd0 Empty2 -> Empty2 > + gen=True success_response=True > command user_def_cmd1 :obj-user_def_cmd1-arg -> None > gen=True success_response=True > command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo Demonstrates another aspect of the arguments/results asymmetry in C and QAPI. At a sufficient level of abstraction, functions take one argument and yield one result. Multiple arguments and results are just notational convenience. Our schema language embraces this: both 'data' and 'returns' are the name of the (single) argument/return type. See the qapi-schema-test.out snipped visible in the patch. The schema also provides syntactic sugar for multiple arguments, so you don't have to define the argument type yourself. It doesn't do that for results. In C, functions take multiple arguments and yield one result. The QAPI code generator "unwraps" its single argument type into multiple C arguments. That's where the "no variants in argument type" restriction comes from. It can't unwrap the return type. That's where the "no sugar for return type" comes from: we really need a C type with a proper name, and ':obj-frobnicate-res' wouldn't be one. Unwrapping the empty argument type results in no arguments: qmp_user_def_cmd0() takes only the additional Error ** argument. Not unwrapping the empty result type results in returning nothing via g_new0(). If that mattered, we could special-case it to return void instead, but it doesn't.
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index f44e01f..7c9cfbf 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -65,7 +65,7 @@ def gen_marshal_vars(arg_type, ret_type): ''', c_type=ret_type.c_type()) - if arg_type: + if arg_type and not arg_type.is_empty(): ret += mcgen(''' QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); QapiDeallocVisitor *qdv; @@ -97,7 +97,7 @@ def gen_marshal_vars(arg_type, ret_type): def gen_marshal_input_visit(arg_type, dealloc=False): ret = '' - if not arg_type: + if not arg_type or arg_type.is_empty(): return ret if dealloc: @@ -177,7 +177,7 @@ def gen_marshal(name, arg_type, ret_type): # 'goto out' produced by gen_marshal_input_visit->gen_visit_members() # for each arg_type member, and by gen_call() for ret_type - if (arg_type and arg_type.members) or ret_type: + if (arg_type and not arg_type.is_empty()) or ret_type: ret += mcgen(''' out: diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c index d6171f2..650ba46 100644 --- a/tests/test-qmp-commands.c +++ b/tests/test-qmp-commands.c @@ -13,6 +13,11 @@ void qmp_user_def_cmd(Error **errp) { } +Empty2 *qmp_user_def_cmd0(Error **errp) +{ + return g_new0(Empty2, 1); +} + void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp) { } diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index b5d0c53..33e8517 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -18,6 +18,8 @@ { 'struct': 'Empty1', 'data': { } } { 'struct': 'Empty2', 'base': 'Empty1', 'data': { } } +{ 'command': 'user_def_cmd0', 'data': 'Empty2', 'returns': 'Empty2' } + # for testing override of default naming heuristic { 'enum': 'QEnumTwo', 'prefix': 'QENUM_TWO', diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 225e2db..6b97fa5 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -203,6 +203,8 @@ command guest-sync :obj-guest-sync-arg -> any gen=True success_response=True command user_def_cmd None -> None gen=True success_response=True +command user_def_cmd0 Empty2 -> Empty2 + gen=True success_response=True command user_def_cmd1 :obj-user_def_cmd1-arg -> None gen=True success_response=True command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
The generator special-cased { 'command':'foo', 'data': {} } to avoid emitting a visitor variable, but failed to see that { 'struct':'NamedEmptyType, 'data': {} } { 'command':'foo', 'data':'NamedEmptyType' } needs the same treatment. There, the generator happily generates a visitor to get no arguments, and a visitor to destroy no arguments; and the compiler isn't happy with that, as demonstrated by the updated qapi-schema-test.json: tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’: tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used [-Werror=unused-but-set-variable] Visitor *v; ^ No change to generated code except for the testsuite addition. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- v2: no change v1: enhance commit message Previously posted as part of qapi cleanup subset E: v9: no change v8: no change v7: no change v6: new patch --- scripts/qapi-commands.py | 6 +++--- tests/test-qmp-commands.c | 5 +++++ tests/qapi-schema/qapi-schema-test.json | 2 ++ tests/qapi-schema/qapi-schema-test.out | 2 ++ 4 files changed, 12 insertions(+), 3 deletions(-)