Re: [PATCH v5 3/9] qapi: make gen_if/gen_endif take a simple string

[Markus Armbruster <armbru@redhat.com>]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Instead of building prepocessor conditions from a list of string, use
> the result generated from QAPISchemaIfCond.cgen().

I understand why you're doing this, but only because I know where you're
headed.  By itself, it is not an improvement: you move C generation out
of common.py into schema.py.  You need to explain why that's useful.

>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi/common.py     | 24 +++++++++++-------------
>  scripts/qapi/gen.py        |  4 ++--
>  scripts/qapi/introspect.py |  4 ++--
>  scripts/qapi/schema.py     |  3 +++
>  scripts/qapi/types.py      | 20 ++++++++++----------
>  scripts/qapi/visit.py      | 12 ++++++------
>  6 files changed, 34 insertions(+), 33 deletions(-)

Missing: qapi-code-gen.txt section "Configuring the schema" has an
example, which needs to be updated.

When the generated code changes, always check the examples, and always
consider describing the change in the commit message.

>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 6ad1eeb61d..c305aaf2f1 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -12,7 +12,7 @@
>  # See the COPYING file in the top-level directory.
>  
>  import re
> -from typing import Match, Optional, Sequence
> +from typing import Match, Optional
>  
>  
>  #: Magic string that gets removed along with all space to its right.
> @@ -194,22 +194,20 @@ def guardend(name: str) -> str:
>                   name=c_fname(name).upper())
>  
>  
> -def gen_if(ifcond: Sequence[str]) -> str:
> -    ret = ''
> -    for ifc in ifcond:
> -        ret += mcgen('''
> +def gen_if(cond: str) -> str:
> +    if not cond:
> +        return ''
> +    return mcgen('''
>  #if %(cond)s
> -''', cond=ifc)
> -    return ret
> +''', cond=cond)
>  
>  
> -def gen_endif(ifcond: Sequence[str]) -> str:
> -    ret = ''
> -    for ifc in reversed(ifcond):
> -        ret += mcgen('''
> +def gen_endif(cond: str) -> str:
> +    if not cond:
> +        return ''
> +    return mcgen('''
>  #endif /* %(cond)s */
> -''', cond=ifc)
> -    return ret
> +''', cond=cond)
>  
>  
>  def must_match(pattern: str, string: str) -> Match[str]:
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 1c5b190276..51a597a025 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -95,9 +95,9 @@ def _wrap_ifcond(ifcond: QAPISchemaIfCond, before: str, after: str) -> str:
>      if added[0] == '\n':
>          out += '\n'
>          added = added[1:]
> -    out += gen_if(ifcond.ifcond)
> +    out += gen_if(ifcond.cgen())
>      out += added
> -    out += gen_endif(ifcond.ifcond)
> +    out += gen_endif(ifcond.cgen())
>      return out
>  
>  
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 77a8c33ad4..474b08fd4d 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -124,10 +124,10 @@ def indent(level: int) -> str:
>          if obj.comment:
>              ret += indent(level) + f"/* {obj.comment} */\n"
>          if obj.ifcond:
> -            ret += gen_if(obj.ifcond.ifcond)
> +            ret += gen_if(obj.ifcond.cgen())
>          ret += _tree_to_qlit(obj.value, level)
>          if obj.ifcond:
> -            ret += '\n' + gen_endif(obj.ifcond.ifcond)
> +            ret += '\n' + gen_endif(obj.ifcond.cgen())
>          return ret
>  
>      ret = ''
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index bc357ebbfa..aa4715c519 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -29,6 +29,9 @@ class QAPISchemaIfCond:
>      def __init__(self, ifcond=None):
>          self.ifcond = ifcond or []
>  
> +    def cgen(self):
> +        return ' && '.join(self.ifcond)

Fragile.  Better:

           return '(' + ') && ('.join(self.ifcond) + ')'

If we want to keep C generation details out of schema.py, we need a
helper mapping self.ifcond: Sequence[str] to C code, similar to how
QAPISchemaEntity.c_name() works with helper c_name().

> +
>      # Returns true if the condition is not void
>      def __bool__(self):
>          return bool(self.ifcond)
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index 3673cf0f49..db9ff95bd1 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -51,13 +51,13 @@ def gen_enum_lookup(name: str,
>  ''',
>                  c_name=c_name(name))
>      for memb in members:
> -        ret += gen_if(memb.ifcond.ifcond)
> +        ret += gen_if(memb.ifcond.cgen())
>          index = c_enum_const(name, memb.name, prefix)
>          ret += mcgen('''
>          [%(index)s] = "%(name)s",
>  ''',
>                       index=index, name=memb.name)
> -        ret += gen_endif(memb.ifcond.ifcond)
> +        ret += gen_endif(memb.ifcond.cgen())
>  
>      ret += mcgen('''
>      },
[More of the same snipped...]