Re: [Qemu-devel] [PATCH v5 02/20] qapi.py: add a simple #ifdef conditional

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Learn to parse #define files provided with -f option, and skip
> undefined #ifdef blocks in the schema.
>
> This is a very simple pre-processing, without stacking support or
> evaluation (it could be implemented if needed).
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi.py | 43 ++++++++++++++++++++++++++++++++++++++++---

Missing: update of docs/qapi-code-gen.txt.

>  1 file changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 21bc32f..d0b8a66 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -76,6 +76,7 @@ struct_types = []
>  union_types = []
>  events = []
>  all_names = {}
> +defs = []
>  
>  #
>  # Parsing the schema into expressions
> @@ -177,6 +178,7 @@ class QAPISchemaParser(object):
>                  self.exprs.append(expr_elem)
>  
>      def accept(self):
> +        ok = True
>          while True:
>              self.tok = self.src[self.cursor]
>              self.pos = self.cursor
> @@ -184,7 +186,19 @@ class QAPISchemaParser(object):
>              self.val = None
>  
>              if self.tok == '#':
> -                self.cursor = self.src.find('\n', self.cursor)
> +                end = self.src.find('\n', self.cursor)
> +                line = self.src[self.cursor:end+1]
> +                self.cursor = end
> +                sline = line.split()
> +                if len(defs) and len(sline) >= 1 \
> +                   and sline[0] in ['ifdef', 'endif']:
> +                    if sline[0] == 'ifdef':
> +                        ok = sline[1] in defs
> +                    elif sline[0] == 'endif':
> +                        ok = True
> +                    continue
> +            elif not ok:
> +                continue
>              elif self.tok in "{}:,[]":
>                  return
>              elif self.tok == "'":

Oww, you're abusing comments!  That's horrible :)

Can we make this real syntax, like everything else, including 'include'?

Unfortunately, the natural

    { 'ifdef': 'CONFIG_FOO'
      'then': ...   # ignored unless CONFIG_FOO
      'else': ...   # ignored if CONFIG_FOO (optional)
    }

is awkward, because the ... have to be a single JSON value, but a QAPI
schema isn't a single JSON value, it's a *sequence* of JSON values.  A
top-level stanza

    JSON-value1 JSON-value2 ...

would become

    [ JSON-value1, JSON-value2, ... ]

within a conditional.  Blech.

Could sacrifice the nesting and do

    { 'ifdef': 'CONFIG_FOO' }
    ...
    { 'endif' }

Widens the gap between syntax and semantics.  Editors can no longer
easily jump over the conditional (e.g. Emacs forward-sexp).  Nested
conditionals becomes as confusing as in C.  Not exactly elegant, either.

Yet another option is to add 'ifdef' keys to the definitions
themselves, i.e.

    { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
      'ifdef': 'TARGET_ARM' }

> @@ -1707,15 +1721,36 @@ def gen_params(arg_type, boxed, extra):
>  #
>  # Common command line parsing
>  #
> +def defile(filename):

>From The Collaborative International Dictionary of English v.0.48 [gcide]:

Defile \De*file"\ (d[-e]*f[imac]l"), v. t. [OE. defoulen,
   -foilen, to tread down, OF. defouler; de- + fouler to trample
   (see Full, v. t.), and OE. defoulen to foul (influenced in
   form by the older verb defoilen). See File to defile,
   Foul, Defoul.]
   1. To make foul or impure; to make filthy; to dirty; to
      befoul; to pollute.
      [1913 Webster]

            They that touch pitch will be defiled. --Shak.
      [1913 Webster]

   2. To soil or sully; to tarnish, as reputation; to taint.
      [1913 Webster]

            He is . . . among the greatest prelates of this age,
            however his character may be defiled by . . . dirty
            hands.                                --Swift.
      [1913 Webster]

   3. To injure in purity of character; to corrupt.
      [1913 Webster]

            Defile not yourselves with the idols of Egypt.
                                                  --Ezek. xx. 7.
      [1913 Webster]

   4. To corrupt the chastity of; to debauch; to violate; to
      rape.
      [1913 Webster]

            The husband murder'd and the wife defiled. --Prior.
      [1913 Webster]

   5. To make ceremonially unclean; to pollute.
      [1913 Webster]

            That which dieth of itself, or is torn with beasts,
            he shall not eat to defile therewith. --Lev. xxii.
                                                  8.
      [1913 Webster]

Fitting in a way; you're defiling the poor, innocent comment syntax ;)

> +    f = open(filename, 'r')
> +    while 1:

while True:

> +        line = f.readline()
> +        if not line:
> +            break
> +        while line[-2:] == '\\\n':
> +            nextline = f.readline()
> +            if not nextline:
> +                break
> +            line = line + nextline
> +        tmp = line.strip()
> +        if tmp[:1] != '#':
> +            continue
> +        tmp = tmp[1:]
> +        words = tmp.split()
> +        if words[0] != "define":
> +            continue
> +        defs.append(words[1])
> +    f.close()

This parses Yet Another Language.  Worse, Yet Another Undocumented
Language.  Why not JSON?

Hmm, peeking ahead to PATCH 04... aha!  This is for reading
config-host.h and config-target.h.  So, this actually doesn't parse
YAUL, it parses C.  Sloppily, of course.

I guess we could instead parse config-host.mak and config-target.mak
sloppily.  Not sure which idea is more disgusting :)

Could we punt evaluating conditionals to the C compiler?  Instead of
emitting TEXT when CONFIG_FOO is defined, emit

    #ifdef CONFIG_FOO
    TEXT
    #endif

>  
>  
>  def parse_command_line(extra_options="", extra_long_options=[]):
>  
>      try:
>          opts, args = getopt.gnu_getopt(sys.argv[1:],
> -                                       "chp:o:" + extra_options,
> +                                       "chp:o:f:" + extra_options,
>                                         ["source", "header", "prefix=",
> -                                        "output-dir="] + extra_long_options)
> +                                        "output-dir=", "--defile="] +

https://docs.python.org/3/library/getopt.html on the third argument:
"The leading '--' characters should not be included in the option name."

> +                                       extra_long_options)
>      except getopt.GetoptError as err:
>          print >>sys.stderr, "%s: %s" % (sys.argv[0], str(err))
>          sys.exit(1)
> @@ -1742,6 +1777,8 @@ def parse_command_line(extra_options="", extra_long_options=[]):
>              do_c = True
>          elif o in ("-h", "--header"):
>              do_h = True
> +        elif o in ("-f", "--defile"):
> +            defile(a)
>          else:
>              extra_opts.append(oa)