Markus Armbruster <arm...@redhat.com> writes: > Marc-André Lureau <marcandre.lur...@redhat.com> writes: > >> Wrap generated code with #if/#endif using an 'ifcontext' on >> QAPIGenCSnippet objects. >> >> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> >> --- >> scripts/qapi/commands.py | 21 ++++++++++++--------- >> tests/test-qmp-cmds.c | 5 +++-- >> 2 files changed, 15 insertions(+), 11 deletions(-) >> >> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py >> index dcc03c7859..72749c7fc5 100644 >> --- a/scripts/qapi/commands.py >> +++ b/scripts/qapi/commands.py >> @@ -239,7 +239,7 @@ class >> QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor): >> QAPISchemaModularCVisitor.__init__( >> self, prefix, 'qapi-commands', >> ' * Schema-defined QAPI/QMP commands', __doc__) >> - self._regy = '' >> + self._regy = QAPIGenCCode() >> self._visited_ret_types = {} >> >> def _begin_module(self, name): >> @@ -275,20 +275,23 @@ class >> QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor): >> void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds); >> ''', >> c_prefix=c_name(self._prefix, protect=False))) >> - genc.add(gen_registry(self._regy, self._prefix)) >> + genc.add(gen_registry(self._regy.get_content(), self._prefix)) >> >> def visit_command(self, name, info, ifcond, arg_type, ret_type, gen, >> success_response, boxed, allow_oob, allow_preconfig): >> if not gen: >> return >> - self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type)) >> - if ret_type and ret_type not in self._visited_ret_types[self._genc]: >> + if ret_type and \ >> + ret_type not in self._visited_ret_types[self._genc]: > > PEP8 prefers parenthesis over backslash. Can touch this up when I > apply. > >> self._visited_ret_types[self._genc].add(ret_type) >> - self._genc.add(gen_marshal_output(ret_type)) >> - self._genh.add(gen_marshal_decl(name)) >> - self._genc.add(gen_marshal(name, arg_type, boxed, ret_type)) >> - self._regy += gen_register_command(name, success_response, >> allow_oob, >> - allow_preconfig) >> + with ifcontext(ret_type.ifcond, self._genh, self._genc, >> self._regy): >> + self._genc.add(gen_marshal_output(ret_type)) >> + with ifcontext(ifcond, self._genh, self._genc, self._regy): >> + self._genh.add(gen_command_decl(name, arg_type, boxed, >> ret_type)) >> + self._genh.add(gen_marshal_decl(name)) >> + self._genc.add(gen_marshal(name, arg_type, boxed, ret_type)) >> + self._regy.add(gen_register_command(name, success_response, >> + allow_oob, allow_preconfig)) > > Okay, now it falls apart differently :) Let's step through the code > real slow. > > The first time we visit a command C returning type T... > > if ret_type and \ > ret_type not in self._visited_ret_types[self._genc]: > self._visited_ret_types[self._genc].add(ret_type) > with ifcontext(ret_type.ifcond, self._genh, self._genc, > self._regy): > self._genc.add(gen_marshal_output(ret_type)) > > ... we generate qmp_marshal_output_T() wrapped in T's condition. > > with ifcontext(ifcond, self._genh, self._genc, self._regy): > self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type)) > self._genh.add(gen_marshal_decl(name)) > self._genc.add(gen_marshal(name, arg_type, boxed, ret_type)) > self._regy.add(gen_register_command(name, success_response, > allow_oob, allow_preconfig)) > > We generate qmp_marshal_C() wrapped in C's condition. This is what > calls qmp_marshal_output_T(). > > If T is a user-defined type, the user is responsible for making this > work, i.e. to make T's condition the conjunction of the T-returning > commands' conditions. > > If T is a built-in type, this isn't possible: the qmp_marshal_output_T() > will be generated unconditionally. > > I append a crude patch to provide test coverage (crude because it > removes coverage of another case). With it applied, make check dies > with > > tests/test-qapi-commands.c:470:13: warning: ‘qmp_marshal_output_str’ > defined but not used [-Wunused-function] > > I think the issue is obscure enough to justify postponing a proper fix, > and just add a FIXME here now. I can do that when I apply.
With the PEP8 tweak and a suitable FIXME Reviewed-by: Markus Armbruster <arm...@redhat.com>