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

[Markus Armbruster]

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.

>>> +    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" ;)

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

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

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