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