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

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Tue, Jul 3, 2018 at 3:37 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>>
>>> Hi
>>>
>>> On Tue, Jul 3, 2018 at 1:53 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
>>>>> ---
>>>>>  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

I'd recommend

      +    if before == after:
      +        return after

If we pass crap, we die in gen_if()'s for ifc in ifcond.  The difference
is now crap includes None.

>>>> 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):
>>>>> +@contextmanager
>>>>> +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

Does that mean I should expect v7 from you?

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