[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers |
Date: |
Tue, 3 Jul 2018 17:08:08 +0200 |
Hi
On Tue, Jul 3, 2018 at 3:37 PM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> Hi
>>
>> On Tue, Jul 3, 2018 at 1:53 PM, Markus Armbruster <address@hidden> wrote:
>>> Marc-André Lureau <address@hidden> writes:
>>>
>>>> Add helpers to wrap generated code with #if/#endif lines.
>>>>
>>>> Add QAPIGenCSnippet class to write C snippet code, make QAPIGenC
>>>> inherit from it, for full C files with copyright headers etc.
>>>
>>> It's called QAPIGenCCode now. Can touch up when I apply, along with
>>> another instance below.
>>>
>>> Leaves the reader wondering *why* you need to splice QAPIGenCCode
>>> between QAPIGen and QAPIGenC. Becomes clear only in PATCH 10.
>>> Providing the class now reduces code churn, but you should explain why.
>>> Perhaps:
>>>
>>> A later patch wants to use QAPIGen for generating C snippets rather
>>> than full C files with copyright headers etc. Splice in class
>>> QAPIGenCCode between QAPIGen and QAPIGenC.
>>
>> sure, thanks
>>
>>>
>>>> Add a 'with' statement context manager that will be used to wrap
>>>> generator visitor methods. The manager will check if code was
>>>> generated before adding #if/#endif lines on QAPIGenCSnippet
>>>> objects. Used in the following patches.
>>>>
>>>> Signed-off-by: Marc-André Lureau <address@hidden>
>>>> ---
>>>> scripts/qapi/common.py | 101 +++++++++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 97 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>>> index 1b56065a80..44eaf25850 100644
>>>> --- a/scripts/qapi/common.py
>>>> +++ b/scripts/qapi/common.py
>>>> @@ -12,6 +12,7 @@
>>>> # See the COPYING file in the top-level directory.
>>>>
>>>> from __future__ import print_function
>>>> +from contextlib import contextmanager
>>>> import errno
>>>> import os
>>>> import re
>>>> @@ -1974,6 +1975,40 @@ def guardend(name):
>>>> name=guardname(name))
>>>>
>>>>
>>>> +def gen_if(ifcond):
>>>> + ret = ''
>>>> + for ifc in ifcond:
>>>> + ret += mcgen('''
>>>> +#if %(cond)s
>>>> +''', cond=ifc)
>>>> + return ret
>>>> +
>>>> +
>>>> +def gen_endif(ifcond):
>>>> + ret = ''
>>>> + for ifc in reversed(ifcond):
>>>> + ret += mcgen('''
>>>> +#endif /* %(cond)s */
>>>> +''', cond=ifc)
>>>> + return ret
>>>> +
>>>> +
>>>> +def wrap_ifcond(ifcond, before, after):
>>>
>>> Looks like a helper method. Name it _wrap_ifcond?
>>
>> Not fond of functions with _. Iirc, it was suggested by a linter to
>> make it a top-level function. I don't mind if you change it on commit.
>
> It's just how Python defines a module's public names. I don't like the
> proliferation of leading underscores either, but I feel it's best to go
> with the flow here.
ok
>
>>>> + if ifcond is None or before == after:
>>>> + return after
>>>
>>> The before == after part suppresses empty conditionals. Suggest
>>>
>>> return after # suppress empty #if ... #endif
>>>
>>> The ifcond is None part is suppresses "non-conditionals". How can this
>>> happen? If it can't, let's drop this part.
>>
>> because the function can be called without additional checks on ifcond
>> is None. I don't see what would be the benefit in moving this to the
>> caller responsibility.
>
> Why stop at None? Why not empty string? Empty containers other than
> []? False? Numeric zero?
>
> To answer my own question: because a precise function signatures reduce
> complexity. "The first argument is a list of strings, no ifs, no buts"
> is simpler than "the first argument may be None, or a list of I don't
> remember what exactly, but if you screw it up, the function hopefully
> throws up before it does real damage" ;)
So you prefer if not ifcond or before == after ? ok
>
>>> Another non-conditional is []. See below.
>>>
>>>> +
>>>> + assert after.startswith(before)
>>>> + out = before
>>>> + added = after[len(before):]
>>>> + if added[0] == '\n':
>>>> + out += '\n'
>>>> + added = added[1:]
>>>
>>> The conditional adjusts vertical space around #if. This might need
>>> tweaking, but let's not worry about that now.
>>>
>>>> + out += gen_if(ifcond)
>>>> + out += added
>>>> + out += gen_endif(ifcond)
>>>
>>> There's no similar adjustment for #endif. Again, let's not worry about
>>> that now.
>>>
>>>> + return out
>>>> +
>>>> +
>>>
>>> Since gen_if([]) and gen_endif([]) return '', wrap_ifcond([], before,
>>> after) returns after. Okay.
>>>
>>>> def gen_enum_lookup(name, values, prefix=None):
>>>> ret = mcgen('''
>>>>
>>>> @@ -2064,6 +2099,7 @@ class QAPIGen(object):
>>>> def __init__(self):
>>>> self._preamble = ''
>>>> self._body = ''
>>>> + self._start_if = None
>>>>
>>>> def preamble_add(self, text):
>>>> self._preamble += text
>>>> @@ -2071,6 +2107,25 @@ class QAPIGen(object):
>>>> def add(self, text):
>>>> self._body += text
>>>>
>>>> + def start_if(self, ifcond):
>>>> + assert self._start_if is None
>>>> +
>>>
>>> Let's drop this blank line.
>>
>> I prefer to have preconditions separated from the code, but ok
>
> I sympathize, but not if both are one-liners.
ok
>
>>>
>>>> + self._start_if = (ifcond, self._body, self._preamble)
>>>
>>> It's now obvious that you can't nest .start_if() ... .endif(). Good.
>>>
>>>> +
>>>> + def _wrap_ifcond(self):
>>>> + pass
>>>> +
>>>> + def end_if(self):
>>>> + assert self._start_if
>>>> +
>>>
>>> Let's drop this blank line.
>>>
>>>> + self._wrap_ifcond()
>>>> + self._start_if = None
>>>> +
>>>> + def get_content(self, fname=None):
>>>> + assert self._start_if is None
>>>> + return (self._top(fname) + self._preamble + self._body
>>>> + + self._bottom(fname))
>>>> +
>>>> def _top(self, fname):
>>>> return ''
>>>>
>>>
>>> Note for later: all this code has no effect unless ._wrap_ifcond() is
>>> overridden.
>>>
>>>> @@ -2091,8 +2146,7 @@ class QAPIGen(object):
>>>> f = open(fd, 'r+', encoding='utf-8')
>>>> else:
>>>> f = os.fdopen(fd, 'r+')
>>>> - text = (self._top(fname) + self._preamble + self._body
>>>> - + self._bottom(fname))
>>>> + text = self.get_content(fname)
>>>> oldtext = f.read(len(text) + 1)
>>>> if text != oldtext:
>>>> f.seek(0)
>>>> @@ -2101,10 +2155,49 @@ class QAPIGen(object):
>>>> f.close()
>>>>
>>>>
>>>> -class QAPIGenC(QAPIGen):
>>>> address@hidden
>>>> +def ifcontext(ifcond, *args):
>>>> + """A 'with' statement context manager to wrap with start_if()/end_if()
>>>>
>>>> - def __init__(self, blurb, pydoc):
>>>> + *args: variable length argument list of QAPIGen
>>>
>>> Perhaps: any number of QAPIGen objects
>>>
>>
>> sure
>>
>>>> +
>>>> + Example::
>>>> +
>>>> + with ifcontext(ifcond, self._genh, self._genc):
>>>> + modify self._genh and self._genc ...
>>>> +
>>>> + Is equivalent to calling::
>>>> +
>>>> + self._genh.start_if(ifcond)
>>>> + self._genc.start_if(ifcond)
>>>> + modify self._genh and self._genc ...
>>>> + self._genh.end_if()
>>>> + self._genc.end_if()
>>>> +
>>>
>>> Can we drop this blank line?
>>>
>>
>> I guess we can, I haven't tested the rst formatting this rigorously.
>> Hopefully the linter does it.
>>
>>>> + """
>>>> + for arg in args:
>>>> + arg.start_if(ifcond)
>>>> + yield
>>>> + for arg in args:
>>>> + arg.end_if()
>>>> +
>>>> +
>>>> +class QAPIGenCCode(QAPIGen):
>>>> +
>>>> + def __init__(self):
>>>> QAPIGen.__init__(self)
>>>> +
>>>> + def _wrap_ifcond(self):
>>>> + self._body = wrap_ifcond(self._start_if[0],
>>>> + self._start_if[1], self._body)
>>>> + self._preamble = wrap_ifcond(self._start_if[0],
>>>> + self._start_if[2], self._preamble)
>>>
>>> Can you explain why you put the machinery for conditionals in QAPIGen
>>> and not QAPIGenCCode?
>>
>> Iirc, this has to do with the fact that QAPIGenDoc is used for
>> visiting, and thus QAPIGen start_if()/end_if() could be handled there,
>
> Can you point me to calls of .start_if(), .end_if(), .get_content() for
> anything but QAPIGenCCode and its subtypes?
>
>> while we only want to generate #ifdef wrapping in QAPIGenCCode. I
>> guess I could move more of start_if()/end_if() data to QAPIGenCCode
>> (make them virtual / 'pass' in QAPIGen) Is that what you would like to
>> see?
>
> If there are no such calls, we should simply move the whole shebang to
> QAPIGenCCode.
done
>
> If there are such calls, the common supertype QAPIGen has to provide the
> methods.
>
> If we expect subtypes other than QAPIGenCCode to implement conditional
> code generation the same way, except for a different ._wrap_ifcond(),
> then the patch is okay as is.
>
> If we don't, the transformation you described looks like an improvement
> to me, because it keeps the actual code closer together.
>
> What's your take on it?
>
> I think I could make the transformation when I apply.
>
>>>
>>>> +
>>>> +
>>>> +class QAPIGenC(QAPIGenCCode):
>>>> +
>>>> + def __init__(self, blurb, pydoc):
>>>> + QAPIGenCCode.__init__(self)
>>>> self._blurb = blurb
>>>> self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
>>>> re.MULTILINE))
>>>
--
Marc-André Lureau