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