Hi
On Tue, Jul 3, 2018 at 5:34 PM, Markus Armbruster <arm...@redhat.com> wrote:
> Marc-André Lureau <marcandre.lur...@gmail.com> writes:
>
>> Hi
>>
>> On Tue, Jul 3, 2018 at 3:37 PM, Markus Armbruster <arm...@redhat.com> wrote:
>>> Marc-André Lureau <marcandre.lur...@gmail.com> writes:
>>>
>>>> Hi
>>>>
>>>> On Tue, Jul 3, 2018 at 1:53 PM, Markus Armbruster <arm...@redhat.com>
>>>> wrote:
>>>>> Marc-André Lureau <marcandre.lur...@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.lur...@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.
>
makes sense, I didn't realize it no longers pass 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?
yes
>
>>> 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
