[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 10/15] qapi/commands: add #if conditions to c
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v6 10/15] qapi/commands: add #if conditions to commands |
Date: |
Tue, 03 Jul 2018 17:09:18 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Markus Armbruster <address@hidden> writes:
> Marc-André Lureau <address@hidden> writes:
>
>> Wrap generated code with #if/#endif using an 'ifcontext' on
>> QAPIGenCSnippet objects.
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>> 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 <address@hidden>