Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> ----- Original Message -----
>> Hi
>>
>> ----- Original Message -----
>> > Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>> >
>> > > Wrap generated code with #if/#endif using the ifcond_decorator.
>> > >
>> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > > ---
>> > > scripts/qapi-commands.py | 2 ++
>> > > tests/test-qmp-commands.c | 4 ++--
>> > > 2 files changed, 4 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
>> > > index 5eb96b2d95..db45415c41 100644
>> > > --- a/scripts/qapi-commands.py
>> > > +++ b/scripts/qapi-commands.py
>> > > @@ -228,6 +228,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>> > > self.defn = None
>> > > self._regy = None
>> > > self._visited_ret_types = None
>> > > + self.if_members = ['decl', 'defn', '_regy']
>> > >
>> > > def visit_begin(self, schema):
>> > > self.decl = ''
>> > > @@ -240,6 +241,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>> > > self._regy = None
>> > > self._visited_ret_types = None
>> > >
>> > > + @ifcond_decorator
>> > > def visit_command(self, name, info, arg_type, ret_type,
>> > > gen, success_response, boxed, ifcond):
>> > > if not gen:
>> > > diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
>> > > index 9b9a7ffee7..ad7b6e4e1d 100644
>> > > --- a/tests/test-qmp-commands.c
>> > > +++ b/tests/test-qmp-commands.c
>> > > @@ -10,11 +10,11 @@
>> > >
>> > > static QmpCommandList qmp_commands;
>> > >
>> > > -/* #if defined(TEST_IF_CMD) */
>> > > +#if defined(TEST_IF_CMD)
>> > > void qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
>> > > {
>> > > }
>> > > -/* #endif */
>> > > +#endif
>> > >
>> > > void qmp_user_def_cmd(Error **errp)
>> > > {
>> >
>> > To find out whether the clever decorator is worth its keep, I replaced
>> > it with the dumbest, most obvious code I could think of (appended).
>> > Does this make visit_command() easier or harder to understand?
>> >
>> >
>> >
>> > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
>> > index db45415c41..13d37d2ac2 100644
>> > --- a/scripts/qapi-commands.py
>> > +++ b/scripts/qapi-commands.py
>> > @@ -228,7 +228,6 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>> > self.defn = None
>> > self._regy = None
>> > self._visited_ret_types = None
>> > - self.if_members = ['decl', 'defn', '_regy']
>> >
>> > def visit_begin(self, schema):
>> > self.decl = ''
>> > @@ -241,18 +240,21 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>> > self._regy = None
>> > self._visited_ret_types = None
>> >
>> > - @ifcond_decorator
>> > def visit_command(self, name, info, arg_type, ret_type,
>> > gen, success_response, boxed, ifcond):
>> > if not gen:
>> > return
>> > - self.decl += gen_command_decl(name, arg_type, boxed, ret_type)
>> > + decl = gen_command_decl(name, arg_type, boxed, ret_type)
>> > + defn = ''
>> > if ret_type and ret_type not in self._visited_ret_types:
>> > self._visited_ret_types.add(ret_type)
>> > - self.defn += gen_marshal_output(ret_type)
>> > - self.decl += gen_marshal_decl(name)
>> > - self.defn += gen_marshal(name, arg_type, boxed, ret_type)
>> > - self._regy += gen_register_command(name, success_response)
>> > + defn += gen_marshal_output(ret_type)
>> > + decl += gen_marshal_decl(name)
>> > + defn += gen_marshal(name, arg_type, boxed, ret_type)
>> > + self.decl += gen_ifcond(ifcond, decl)
>> > + self.defn += gen_ifcond(ifcond, defn)
>> > + self._regy += gen_ifcond(ifcond,
>> > + gen_register_command(name,
>> > success_response))
>>
>> This is one of the most simple case, and it is already very intrusive. If you
>> have more branches / early return it becomes awful.
>>
>> Do it for all the places where we have @ifcond_decorator and you will
>> probably prefer the concise decorated version. For me this case is one of
>> the reason why decorators exist: to wrap enter/exit of a function.
This is just a quick experiment. We certainly have to compare complete
solutions to decide whether the decorator is worthwhile.
> Oh and your version looks inferior, since it will generate empty #if/#endif blocks. You seemed to be quite strict about having the generated output be nicely formatted & human friendly, so I thought clearing those empty block was a must for you.
Yes, it is a must for me. Should be easy enough to fix: in
gen_ifcond(), replace "if ifcond" by "if ifcond and text is not blank".
>> >
>> >
>> > (input_file, output_dir, do_c, do_h, prefix, opts) = parse_command_line()
>> > diff --git a/scripts/qapi.py b/scripts/qapi.py
>> > index 52099332f1..3de5b8b8f0 100644
>> > --- a/scripts/qapi.py
>> > +++ b/scripts/qapi.py
>> > @@ -1918,29 +1918,10 @@ def gen_endif(ifcond):
>> > return ret
>> >
>> >
>> > -# Wrap a method to add #if / #endif to generated code, only if some
>> > -# code was generated.
>> > -# self must have 'if_members' listing the attributes to wrap.
>> > -# The last argument of the wrapped function must be the 'ifcond'.
>> > -def ifcond_decorator(func):
>> > -
>> > - def func_wrapper(self, *args, **kwargs):
>> > - ifcond = args[-1]
>> > - save = {}
>> > - for mem in self.if_members:
>> > - save[mem] = getattr(self, mem)
>> > - func(self, *args, **kwargs)
>> > - for mem, val in save.items():
>> > - newval = getattr(self, mem)
>> > - if newval != val:
>> > - assert newval.startswith(val)
>> > - newval = newval[len(val):]
>> > - val += gen_if(ifcond)
>> > - val += newval
>> > - val += gen_endif(ifcond)
>> > - setattr(self, mem, val)
>> > -
>> > - return func_wrapper
>> > +def gen_ifcond(ifcond, ctext):
>> > + if ifcond:
>> > + return gen_if(ifcond) + ctext + gen_endif(ifcond)
>> > + return ctext
>> >
>> >
>> > def gen_enum_lookup(name, values, prefix=None):
>> >
>>