Re: [Qemu-devel] [PATCH v3 08/49] qapi: add #if/#endif helpers

[Markus Armbruster]

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.
>
> 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 | 82 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 78 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 47efe79758..60c1d0a783 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
> @@ -1964,6 +1965,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?

> +    if ifcond is None or before == after:
> +        return after

The before == after part suppresses empty conditionals.  We'll see later
how we end up with them.

The ifcond is None part is suppresses "non-conditionals".  I wonder how
that can happen.

Another non-conditional is [].  Can that happen?

Questions like these indicate the function needs a contract.

> +
> +    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
> +
> +
>  def gen_enum_lookup(name, values, prefix=None):
>      ret = mcgen('''
>  
> @@ -2054,6 +2089,7 @@ class QAPIGen(object):
>      def __init__(self):
>          self._preamble = ''
>          self._body = ''
> +        self._ifcond = None
>  
>      def preamble_add(self, text):
>          self._preamble += text
> @@ -2061,6 +2097,23 @@ class QAPIGen(object):
>      def add(self, text):
>          self._body += text
>  
> +    def start_if(self, ifcond):

What are @ifcond's legal values?  In particular, is it okay to pass
None?

If not, then we have an easy way to check whether we're between
.start_if() and .end_if(): ._ifcond is not None.

> +        self._ifcond = ifcond

What if self._ifcond is not None?

Can happen only when you call .start_if within .start_if() ... .endif().
If that's not supposed to work, let's assert self._ifcond is None here,
and add a function contract spelling out the restriction.

> +        self._start_if_body = self._body
> +        self._start_if_preamble = self._preamble
> +
> +    def _wrap_ifcond(self):
> +        pass
> +
> +    def end_if(self):

If .start_if() doesn't take None, then we can catch misuse of .end_if()
by asserting ._ifcond is not None here.

> +        self._wrap_ifcond()
> +        self._ifcond = None

I'd zap ._start_if_body and ._start_if_preamble here.

Note for later: use of .start_if() ... .end_if() has no effect unless
._wrap_ifcond() is overridden.

> +
> +    def get_content(self, fname=None):
> +        assert self._ifcond is None
> +        return (self._top(fname) + self._preamble + self._body
> +                + self._bottom(fname))
> +
>      def _top(self, fname):
>          return ''
>  
> @@ -2078,8 +2131,7 @@ class QAPIGen(object):
>                      raise
>          fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
>          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)
> @@ -2088,10 +2140,32 @@ class QAPIGen(object):
>          f.close()
>  
>  
> -class QAPIGenC(QAPIGen):
> address@hidden
> +def ifcontext(ifcond, *args):
> +    saved = []
> +    for arg in args:
> +        arg.start_if(ifcond)
> +    yield
> +    for arg in args:
> +        arg.end_if()

@saved is write-only.

How to use the function isn't immediately obvious[*]; I had to glance at
later patches to be sure.  Please add a function comment.

>  
> -    def __init__(self, blurb, pydoc):
> +
> +class QAPIGenCSnippet(QAPIGen):
> +
> +    def __init__(self):
>          QAPIGen.__init__(self)
> +
> +    def _wrap_ifcond(self):
> +        self._body = wrap_ifcond(self._ifcond,
> +                                 self._start_if_body, self._body)
> +        self._preamble = wrap_ifcond(self._ifcond,
> +                                     self._start_if_preamble, self._preamble)
> +
> +
> +class QAPIGenC(QAPIGenCSnippet):
> +
> +    def __init__(self, blurb, pydoc):
> +        QAPIGenCSnippet.__init__(self)
>          self._blurb = blurb
>          self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
>                                                    re.MULTILINE))

Why do you need the intermediary class QAPIGenCSnippet?

As far as I can see, only one subclass overrides ._wrap_ifcond().  Why
is the general machinery in the superclass, where it does nothing, and
not in the subclass that makes it do something?


[*] Compared to the decorator you had in v4, it's simple as pie.
There *had* to be a better way, and it looks like you found one.
Appreciated!