Re: [Qemu-devel] [PATCH v3 09/50] qapi: add #if/#endif helpers

[Marc-André Lureau]

Hi

On Thu, Dec 7, 2017 at 3:10 PM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> Add helpers to wrap generated code with #if/#endif lines.
>>
>> Add a function decorator that will be used to wrap visitor methods.
>> The decorator will check if code was generated before adding #if/#endif
>> lines. Used in the following patches.
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  scripts/qapi.py | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 2a8e60e975..94b735d8d6 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -1897,6 +1897,61 @@ def guardend(name):
>>                   name=guardname(name))
>>
>>
>> +def gen_if(ifcond):
>> +    if not ifcond:
>> +        return ''
>> +    if isinstance(ifcond, str):
>> +        ifcond = [ifcond]
>
> Perhaps we should take this normalization step in the QAPISchema
> constructors.

Yes, it's not very convenient though, I added a TODO note

>
>> +    ret = ''
>> +    for ifc in ifcond:
>> +        ret += mcgen('''
>> +#if %(cond)s
>> +''', cond=ifc)
>> +    return ret
>> +
>> +
>> +def gen_endif(ifcond):
>> +    if not ifcond:
>> +        return ''
>> +    if isinstance(ifcond, str):
>> +        ifcond = [ifcond]
>> +    ret = ''
>> +    for ifc in reversed(ifcond):
>> +        ret += mcgen('''
>> +#endif /* %(cond)s */
>> +''', cond=ifc)
>> +    return ret
>> +
>> +
>> +# Wrap a method to add #if / #endif to generated code, only if some
>> +# code was generated. The method must have an 'ifcond' argument.
>> +# self must have 'if_members' listing the attributes to wrap.
>> +def ifcond_decorator(func):
>> +
>> +    def func_wrapper(self, *args, **kwargs):
>> +        import inspect
>
> Is hiding imports in function a good idea?

I believe it's best to restrict the import to the scope it is being
used, especially if it's very specific to that place. Some code do
that already.

>
>> +        idx = inspect.getargspec(func).args.index('ifcond')
>> +        ifcond = args[idx - 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):]
>> +                if newval[0] == '\n':
>> +                    val += '\n'
>> +                    newval = newval[1:]
>> +                val += gen_if(ifcond)
>> +                val += newval
>> +                val += gen_endif(ifcond)
>> +            setattr(self, mem, val)
>> +
>> +    return func_wrapper
>> +
>> +
>>  def gen_enum_lookup(name, values, prefix=None):
>>      ret = mcgen('''
>
> My gut feeling is still "too clever by half", but i'm reserving
> judgement until after review of its use, and exploration of
> alternatives.
>

Could easily be done as a follow-up.

thanks

-- 
Marc-André Lureau