Re: [PATCH v3 2/9] qapi: move gen_if/gen_endif to QAPISchemaIfCond

[John Snow <jsnow@redhat.com>]

On 4/29/21 9:40 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Move the generating function to the QAPISchemaIfCond class.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   scripts/qapi/common.py     | 20 +-------------------
>   scripts/qapi/gen.py        |  6 ++----
>   scripts/qapi/introspect.py | 11 +++--------
>   scripts/qapi/schema.py     | 18 +++++++++++++++++-
>   scripts/qapi/types.py      | 28 +++++++++++-----------------
>   scripts/qapi/visit.py      | 14 ++++++--------
>   6 files changed, 40 insertions(+), 57 deletions(-)
> 
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index cbd3fd81d3..b7f475a160 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 Optional, Sequence
> +from typing import Optional
>   
>   
>   #: Magic string that gets removed along with all space to its right.
> @@ -192,21 +192,3 @@ def guardend(name: str) -> str:
>   #endif /* %(name)s */
>   ''',
>                    name=c_fname(name).upper())
> -
> -
> -def gen_if(ifcond: Sequence[str]) -> str:
> -    ret = ''
> -    for ifc in ifcond:
> -        ret += mcgen('''
> -#if %(cond)s
> -''', cond=ifc)
> -    return ret
> -
> -
> -def gen_endif(ifcond: Sequence[str]) -> str:
> -    ret = ''
> -    for ifc in reversed(ifcond):
> -        ret += mcgen('''
> -#endif /* %(cond)s */
> -''', cond=ifc)
> -    return ret
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 1c5b190276..ab26d5c937 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -24,8 +24,6 @@
>   from .common import (
>       c_fname,
>       c_name,
> -    gen_endif,
> -    gen_if,
>       guardend,
>       guardstart,
>       mcgen,
> @@ -95,9 +93,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 += ifcond.gen_if()
>       out += added
> -    out += gen_endif(ifcond.ifcond)
> +    out += ifcond.gen_endif()
>       return out
>   
>   
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 77a8c33ad4..a2a8a57b9a 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -22,12 +22,7 @@
>       Union,
>   )
>   
> -from .common import (
> -    c_name,
> -    gen_endif,
> -    gen_if,
> -    mcgen,
> -)
> +from .common import c_name, mcgen
>   from .gen import QAPISchemaMonolithicCVisitor
>   from .schema import (
>       QAPISchema,
> @@ -124,10 +119,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 += obj.ifcond.gen_if()
>           ret += _tree_to_qlit(obj.value, level)
>           if obj.ifcond:
> -            ret += '\n' + gen_endif(obj.ifcond.ifcond)
> +            ret += '\n' + obj.ifcond.gen_endif()
>           return ret
>   
>       ret = ''
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 7d6f390fa6..8e6d0a5296 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, mcgen
>   from .error import QAPISemError, QAPISourceError
>   from .expr import check_exprs
>   from .parser import QAPISchemaParser
> @@ -29,6 +29,22 @@ class QAPISchemaIfCond:
>       def __init__(self, ifcond=None):
>           self.ifcond = ifcond or []
>   
> +    def gen_if(self):
> +        ret = ''
> +        for ifc in self.ifcond:
> +            ret += mcgen('''
> +#if %(cond)s
> +''', cond=ifc)
> +        return ret
> +
> +    def gen_endif(self):
> +        ret = ''
> +        for ifc in reversed(self.ifcond):
> +            ret += mcgen('''
> +#endif /* %(cond)s */
> +''', cond=ifc)
> +        return ret
> +
>       def __bool__(self):
>           return bool(self.ifcond)
>   
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index 3673cf0f49..831294fe42 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -15,13 +15,7 @@
>   
>   from typing import List, Optional
>   
> -from .common import (
> -    c_enum_const,
> -    c_name,
> -    gen_endif,
> -    gen_if,
> -    mcgen,
> -)
> +from .common import c_enum_const, c_name, mcgen
>   from .gen import QAPISchemaModularCVisitor, ifcontext
>   from .schema import (
>       QAPISchema,
> @@ -51,13 +45,13 @@ def gen_enum_lookup(name: str,
>   ''',
>                   c_name=c_name(name))
>       for memb in members:
> -        ret += gen_if(memb.ifcond.ifcond)
> +        ret += memb.ifcond.gen_if()
>           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 += memb.ifcond.gen_endif()
>   
>       ret += mcgen('''
>       },
> @@ -81,12 +75,12 @@ def gen_enum(name: str,
>                   c_name=c_name(name))
>   
>       for memb in enum_members:
> -        ret += gen_if(memb.ifcond.ifcond)
> +        ret += memb.ifcond.gen_if()
>           ret += mcgen('''
>       %(c_enum)s,
>   ''',
>                        c_enum=c_enum_const(name, memb.name, prefix))
> -        ret += gen_endif(memb.ifcond.ifcond)
> +        ret += memb.ifcond.gen_endif()
>   
>       ret += mcgen('''
>   } %(c_name)s;
> @@ -126,7 +120,7 @@ def gen_array(name: str, element_type: QAPISchemaType) -> str:
>   def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) -> str:
>       ret = ''
>       for memb in members:
> -        ret += gen_if(memb.ifcond.ifcond)
> +        ret += memb.ifcond.gen_if()
>           if memb.optional:
>               ret += mcgen('''
>       bool has_%(c_name)s;
> @@ -136,7 +130,7 @@ def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) -> str:
>       %(c_type)s %(c_name)s;
>   ''',
>                        c_type=memb.type.c_type(), c_name=c_name(memb.name))
> -        ret += gen_endif(memb.ifcond.ifcond)
> +        ret += memb.ifcond.gen_endif()
>       return ret
>   
>   
> @@ -159,7 +153,7 @@ def gen_object(name: str, ifcond: QAPISchemaIfCond,
>       ret += mcgen('''
>   
>   ''')
> -    ret += gen_if(ifcond.ifcond)
> +    ret += ifcond.gen_if()
>       ret += mcgen('''
>   struct %(c_name)s {
>   ''',
> @@ -193,7 +187,7 @@ def gen_object(name: str, ifcond: QAPISchemaIfCond,
>       ret += mcgen('''
>   };
>   ''')
> -    ret += gen_endif(ifcond.ifcond)
> +    ret += ifcond.gen_endif()
>   
>       return ret
>   
> @@ -220,13 +214,13 @@ def gen_variants(variants: QAPISchemaVariants) -> str:
>       for var in variants.variants:
>           if var.type.name == 'q_empty':
>               continue
> -        ret += gen_if(var.ifcond.ifcond)
> +        ret += var.ifcond.gen_if()
>           ret += mcgen('''
>           %(c_type)s %(c_name)s;
>   ''',
>                        c_type=var.type.c_unboxed_type(),
>                        c_name=c_name(var.name))
> -        ret += gen_endif(var.ifcond.ifcond)
> +        ret += var.ifcond.gen_endif()
>   
>       ret += mcgen('''
>       } u;
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 67721b2470..9d9196a143 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -18,8 +18,6 @@
>   from .common import (
>       c_enum_const,
>       c_name,
> -    gen_endif,
> -    gen_if,
>       indent,
>       mcgen,
>   )
> @@ -79,7 +77,7 @@ def gen_visit_object_members(name: str,
>   
>       for memb in members:
>           deprecated = 'deprecated' in [f.name for f in memb.features]
> -        ret += gen_if(memb.ifcond.ifcond)
> +        ret += memb.ifcond.gen_if()
>           if memb.optional:
>               ret += mcgen('''
>       if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
> @@ -112,7 +110,7 @@ def gen_visit_object_members(name: str,
>               ret += mcgen('''
>       }
>   ''')
> -        ret += gen_endif(memb.ifcond.ifcond)
> +        ret += memb.ifcond.gen_endif()
>   
>       if variants:
>           tag_member = variants.tag_member
> @@ -126,7 +124,7 @@ def gen_visit_object_members(name: str,
>           for var in variants.variants:
>               case_str = c_enum_const(tag_member.type.name, var.name,
>                                       tag_member.type.prefix)
> -            ret += gen_if(var.ifcond.ifcond)
> +            ret += var.ifcond.gen_if()
>               if var.type.name == 'q_empty':
>                   # valid variant and nothing to do
>                   ret += mcgen('''
> @@ -142,7 +140,7 @@ def gen_visit_object_members(name: str,
>                                case=case_str,
>                                c_type=var.type.c_name(), c_name=c_name(var.name))
>   
> -            ret += gen_endif(var.ifcond.ifcond)
> +            ret += var.ifcond.gen_endif()
>           ret += mcgen('''
>       default:
>           abort();
> @@ -228,7 +226,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
>                   c_name=c_name(name))
>   
>       for var in variants.variants:
> -        ret += gen_if(var.ifcond.ifcond)
> +        ret += var.ifcond.gen_if()
>           ret += mcgen('''
>       case %(case)s:
>   ''',
> @@ -254,7 +252,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
>           ret += mcgen('''
>           break;
>   ''')
> -        ret += gen_endif(var.ifcond.ifcond)
> +        ret += var.ifcond.gen_endif()
>   
>       ret += mcgen('''
>       case QTYPE_NONE:
> 

Tested-by: John Snow <jsnow@redhat.com>

Seems fine, though I'm a lot less sure of baking the C-specific stuff 
right into the class -- I want a bit more distance to the C output 
instead of less.

(Though I do admit that I'm quite fond of centralizing things into 
classes like this myself. I think if we want to add Rust, Go, Python and 
other generators that it won't scale the way we want it to.)

--js