Re: [PATCH v7 04/10] qapi: introduce QAPISchemaIfCond.cgen()

[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() and hide the
> implementation details.
>
> Note: this patch introduces a minor regression, generating a redundant
> pair of parenthesis. This is fixed in a later patch in this
> series ("qapi: replace if condition list with dict [..]")

Fixed in most, but not all instances, actually.  I can see some 50
remaining in generated qapi-*.[ch] at the end of this series.  Let's
s/fixed/mostly fixed/, and move on.

>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi/common.py     | 35 ++++++++++++++++++++++-------------
>  scripts/qapi/gen.py        |  4 ++--
>  scripts/qapi/introspect.py |  4 ++--
>  scripts/qapi/schema.py     |  5 ++++-
>  scripts/qapi/types.py      | 20 ++++++++++----------
>  scripts/qapi/visit.py      | 12 ++++++------
>  6 files changed, 46 insertions(+), 34 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 6ad1eeb61d..ba9fe14e4b 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -12,7 +12,12 @@
>  # See the COPYING file in the top-level directory.
>  
>  import re
> -from typing import Match, Optional, Sequence
> +from typing import (
> +    List,
> +    Match,
> +    Optional,
> +    Union,
> +)
>  
>  
>  #: Magic string that gets removed along with all space to its right.
> @@ -194,22 +199,26 @@ 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 cgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> +    if not ifcond:
> +        return ''
> +    return '(' + ') && ('.join(ifcond) + ')'
> +
> +
> +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 e23725e2f9..bd4233ecee 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.is_present():
> -            ret += gen_if(obj.ifcond.ifcond)
> +            ret += gen_if(obj.ifcond.cgen())
>          ret += _tree_to_qlit(obj.value, level)
>          if obj.ifcond.is_present():
> -            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 24caa4ad43..f018cfc511 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -19,7 +19,7 @@
>  import re
>  from typing import Optional
>  
> -from .common import POINTER_SUFFIX, c_name
> +from .common import POINTER_SUFFIX, c_name, cgen_ifcond
>  from .error import QAPIError, QAPISemError, QAPISourceError
>  from .expr import check_exprs
>  from .parser import QAPISchemaParser
> @@ -29,6 +29,9 @@ class QAPISchemaIfCond:
>      def __init__(self, ifcond=None):
>          self.ifcond = ifcond or []
>  
> +    def cgen(self):
> +        return cgen_ifcond(self.ifcond)
> +
>      def is_present(self):
>          return bool(self.ifcond)
>  

Possible improvement: have methods gen_if() and gen_endif(), so we can
replace

    gen_if(IFCOND.cgen())

and

    gen_endif(IFCOND.cgen())

by

    IFCOND.gen_if()

and

    IFCOND.gen_endif()

Can be done on top.

[...]