[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 14:35:14 +0200 |
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