marcandre.lur...@redhat.com writes: > From: Marc-André Lureau <marcandre.lur...@redhat.com> > > The generated code doesn't quite handle the conditional arguments. > For example, 'bar' in 'test-if-cmd' is not correctly surrounded by #if > conditions. See generated code in qmp_marshal_test_if_cmd(). > > Note that if there are multiple optional arguments at the last position, > there might be compilation issues due to extra comas. I left an assert > and FIXME for later. > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > --- > scripts/qapi/commands.py | 4 ++++ > scripts/qapi/gen.py | 19 ++++++++++++++----- > scripts/qapi/visit.py | 2 ++ > tests/qapi-schema/qapi-schema-test.json | 3 ++- > 4 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py > index 79c5e5c3a9..07997d1586 100644 > --- a/scripts/qapi/commands.py > +++ b/scripts/qapi/commands.py > @@ -64,9 +64,13 @@ def gen_call(name: str, > elif arg_type: > assert not arg_type.variants > for memb in arg_type.members: > + if memb.ifcond.is_present(): > + argstr += '\n' + memb.ifcond.gen_if() > if memb.need_has(): > argstr += 'arg.has_%s, ' % c_name(memb.name) > argstr += 'arg.%s, ' % c_name(memb.name) > + if memb.ifcond.is_present(): > + argstr += '\n' + memb.ifcond.gen_endif() > > lhs = '' > if ret_type:
@argstr is emitted further down: %(lhs)sqmp_%(name)s(%(args)s&err); ''', name=name, args=argstr, lhs=lhs) ret += mcgen(''' if (err) { ''') Before the patch, @argstr contains no newlines. Works. After the patch, it may contain newlines, and if it does, intentation is messed up. For instance, in the code generated for qapi-schema-test.json: retval = qmp_test_if_cmd(arg.foo, #if defined(TEST_IF_CMD_BAR) arg.bar, #endif /* defined(TEST_IF_CMD_BAR) */ &err); Strings interpolated into the mcgen() argument should not contain newlines. I'm afraid you have to rewrite the code emitting the call. > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py > index b5a8d03e8e..ba57e72c9b 100644 > --- a/scripts/qapi/gen.py > +++ b/scripts/qapi/gen.py > @@ -111,22 +111,31 @@ def build_params(arg_type: > Optional[QAPISchemaObjectType], > boxed: bool, > extra: Optional[str] = None) -> str: > ret = '' > - sep = '' > if boxed: > assert arg_type > ret += '%s arg' % arg_type.c_param_type() > - sep = ', ' > + if extra: > + ret += ', ' > elif arg_type: > assert not arg_type.variants > + n = 0 > for memb in arg_type.members: > - ret += sep > - sep = ', ' > + n += 1 > + if memb.ifcond.is_present(): > + ret += '\n' + memb.ifcond.gen_if() > if memb.need_has(): > ret += 'bool has_%s, ' % c_name(memb.name) > ret += '%s %s' % (memb.type.c_param_type(), > c_name(memb.name)) > + if extra or n != len(arg_type.members): > + ret += ', ' > + else: > + # FIXME: optional last argument may break compilation > + assert not memb.ifcond.is_present() Does the assertion guard against the C compilation failure? Is it possible to write schema code that triggers it? > + if memb.ifcond.is_present(): > + ret += '\n' + memb.ifcond.gen_endif() > if extra: > - ret += sep + extra > + ret += extra > return ret if ret else 'void' > > Same newline issue as in gen_call(). Generated code: UserDefThree *qmp_test_if_cmd(TestIfStruct *foo, #if defined(TEST_IF_CMD_BAR) TestIfEnum bar, #endif /* defined(TEST_IF_CMD_BAR) */ Error **errp); > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py > index 26a584ee4c..c56ea4d724 100644 > --- a/scripts/qapi/visit.py > +++ b/scripts/qapi/visit.py > @@ -74,11 +74,13 @@ def gen_visit_object_members(name: str, > sep = '' > for memb in members: > if memb.optional and not memb.need_has(): > + ret += memb.ifcond.gen_if() > ret += mcgen(''' > bool has_%(c_name)s = !!obj->%(c_name)s; > ''', > c_name=c_name(memb.name)) > sep = '\n' > + ret += memb.ifcond.gen_endif() > ret += sep > > if base: This hunk has no effect on the code generated for our schemas as far as I can tell. Is it superfluous? Incorrect? Gap in test coverage? Or am I confused? > diff --git a/tests/qapi-schema/qapi-schema-test.json > b/tests/qapi-schema/qapi-schema-test.json > index ba7302f42b..baa4e69f63 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -258,7 +258,8 @@ > > { 'event': 'TEST_IF_EVENT', > 'data': { 'foo': 'TestIfStruct', > - 'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } }, > + 'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' }, > + 'baz': 'int' }, > 'if': { 'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT'] } } > > { 'event': 'TEST_IF_EVENT2', 'data': {},