Re: [Qemu-devel] [PATCH 07/26] qapi: add 'if' condition on top-level schema elements

[Eduardo Habkost]

On Tue, Aug 22, 2017 at 06:52:19PM +0200, Markus Armbruster wrote:
> Marc-André Lureau <address@hidden> writes:
[...]
> >>> +    def func_wrapper(self, *args, **kwargs):
> >>> +        funcname = self.__class__.__name__ + '.' + func.__name__
> >>> +        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, funcname)
> >>
> >> Emitting comments pointing to the QAPI schema into the generated code is
> >> often helpful.  But this one points to QAPI generator code.  Why is that
> >> useful?
> >
> > That was mostly useful during development, removed
> >
> >>
> >>> +                val += newval
> >>> +                val += gen_endif(ifcond, funcname)
> >>> +            setattr(self, mem, val)
> >>> +
> >>> +    return func_wrapper
> >>> +
> >>
> >> pep8 again:
> >>
> >>     scripts/qapi.py:1955:1: E302 expected 2 blank lines, found 1
> >>
> >> Peeking ahead: this function is used as a decorator.  Let's help the
> >> reader and mention that in the function comment, or by naming the
> >> function suitably.  ifcond_decorator?
> >
> > done
> >
> >>
> >> Messing with the wrapped method's class's attributes is naughty.  Worse,
> >> it's hard to understand.  What alternatives have you considered?
> >
> > Well, I started writing the code that checked if members got code
> > added, I had to put some enter()/leave() code everywhere. Then I
> > realize this could easily be the job for a decorator. I think the end
> > result is pretty neat.
> 
> I think "clever" would describe it better than "neat".  Possibly "too
> clever".

FWIW, I was investigating something else in the series and took a
while to understand how the #if lines were magically appearing on
self.decl and self.defn.

I'd prefer something simpler like:

    def cond(ifcond, s):
        if s:
            return gen_if(ifcond) + s + gen_endif(ifcond)
        return s

    def visit_command(self, name, info, arg_type, ret_type,
                      gen, success_response, boxed, ifcond):
        if not gen:
            return
        self.decl += cond(ifcond, gen_command_decl(name, arg_type, boxed, 
ret_type))
        if ret_type and ret_type not in self._visited_ret_types:
            self._visited_ret_types.add(ret_type)
            self.defn += cond(ifcond,gen_marshal_output(ret_type))
        self.decl += cond(ifcond, gen_marshal_decl(name))
        self.defn += cond(ifcond, gen_marshal(name, arg_type, boxed, ret_type))
        self._regy += cond(ifcond, gen_register_command(name, success_response))

If all callers of some of the gen_*() functions above are wrapped
by ifcond(), then a decorator around them could still be useful.
But preferably if the decorator affects only the function
arguments and return value, not messing with the object
attributes or other state outside the function.

-- 
Eduardo