Re: [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers

[Marc-André Lureau]

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.

>
>> +    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.

>
> 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

>
>> +        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,
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?

>
>> +
>> +
>> +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