Hi

On Mon, Aug 2, 2021 at 6:46 PM Markus Armbruster <armbru@redhat.com> wrote:

> 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.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Please mention that the patch changes generated code.  See below for
> details.
>

I'll add

    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 [..]")


> > ---
> >  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)
> >
> >
>
> This patch does three things:
>
> (1) Change gen_if(), gen_endif() to always generate a single #if,
>     #endif.  This enables:
>
> (2) Factor cgen_ifcond() out of gen_if() and gen_endif()
>
> (3) Lift the call of cgen_ifcond() into into gen_if()'s, gen_endif()'s
>     callers.
>
> I'd split the patch.  This is *not* a demand.
>
> The motivation for (3) is unclear.  Is it so gen_if() doesn't depend on
> QAPISchemaIfCond?
>
> Step (1) affects the generated code.  When @ifcond is [COND1, COND2, ...],
> gen_if()'s value changes from
>
>     #if COND1
>     #if COND2
>     ...
>
> to
>
>     #if (COND1) && (COND2)
>
> Example: in tests/test-qapi-introspect.c
>
>     @@ -259,11 +257,9 @@ const QLitObject test_qmp_schema_qlit =
>          QLIT_QDICT(((QLitDictEntry[]) {
>              { "arg-type", QLIT_QSTR("1"), },
>              { "features", QLIT_QLIST(((QLitObject[]) {
>     -#if defined(TEST_IF_COND_1)
>     -#if defined(TEST_IF_COND_2)
>     +#if (defined(TEST_IF_COND_1)) && (defined(TEST_IF_COND_2))
>                  QLIT_QSTR("feature1"),
>     -#endif /* defined(TEST_IF_COND_2) */
>     -#endif /* defined(TEST_IF_COND_1) */
>     +#endif /* (defined(TEST_IF_COND_1)) && (defined(TEST_IF_COND_2)) */
>                  {}
>              })), },
>              { "meta-type", QLIT_QSTR("command"), },
>
> The common case: when it's just [COND], the value changes from
>
>     #if COND
>
> to
>
>     #if (COND)
>
> which is a bit ugly.
>
> Example: in qapi-types-block-export.c
>
>     @@ -76,7 +76,7 @@ const QEnumLookup FuseExportAllowOther_l
>          .size = FUSE_EXPORT_ALLOW_OTHER__MAX
>      };
>
>     -#if defined(CONFIG_FUSE)
>     +#if (defined(CONFIG_FUSE))
>      void qapi_free_BlockExportOptionsFuse(BlockExportOptionsFuse *obj)
>      {
>          Visitor *v;
>     @@ -89,7 +89,7 @@ void qapi_free_BlockExportOptionsFuse(Bl
>          visit_type_BlockExportOptionsFuse(v, NULL, &obj, NULL);
>          visit_free(v);
>      }
>     -#endif /* defined(CONFIG_FUSE) */
>     +#endif /* (defined(CONFIG_FUSE)) */
>
>      void qapi_free_NbdServerAddOptions(NbdServerAddOptions *obj)
>      {
>
> Avoiding the redundant pair of parenthesis takes another special case.
> Let's do it.
>
> >  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 c35fa3bf51..70120f0dcc 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)
> > +
>
> As far as I can tell, this is only ever used like
>
>        gen_if(obj.ifcond.cgen())
>
> and
>
>        gen_endif(obj.ifcond.cgen())
>
> Wouldn't
>
>        obj.ifcond.gen_if()
>
> and
>
>        obj.ifcond.gen_endif()
>
> be neater?
>

Yes


> Not a demand, since we can get there on top if we want to.
>
> >      def is_present(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('''
> >      },
> > @@ -81,12 +81,12 @@ def gen_enum(name: str,
> >                  c_name=c_name(name))
> >
> >      for memb in enum_members:
> > -        ret += gen_if(memb.ifcond.ifcond)
> > +        ret += gen_if(memb.ifcond.cgen())
> >          ret += mcgen('''
> >      %(c_enum)s,
> >  ''',
> >                       c_enum=c_enum_const(name, memb.name, prefix))
> > -        ret += gen_endif(memb.ifcond.ifcond)
> > +        ret += gen_endif(memb.ifcond.cgen())
> >
> >      ret += mcgen('''
> >  } %(c_name)s;
> > @@ -126,7 +126,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 += gen_if(memb.ifcond.cgen())
> >          if memb.optional:
> >              ret += mcgen('''
> >      bool has_%(c_name)s;
> > @@ -136,7 +136,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 += gen_endif(memb.ifcond.cgen())
> >      return ret
> >
> >
> > @@ -159,7 +159,7 @@ def gen_object(name: str, ifcond: QAPISchemaIfCond,
> >      ret += mcgen('''
> >
> >  ''')
> > -    ret += gen_if(ifcond.ifcond)
> > +    ret += gen_if(ifcond.cgen())
> >      ret += mcgen('''
> >  struct %(c_name)s {
> >  ''',
> > @@ -193,7 +193,7 @@ def gen_object(name: str, ifcond: QAPISchemaIfCond,
> >      ret += mcgen('''
> >  };
> >  ''')
> > -    ret += gen_endif(ifcond.ifcond)
> > +    ret += gen_endif(ifcond.cgen())
> >
> >      return ret
> >
> > @@ -220,13 +220,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 += gen_if(var.ifcond.cgen())
> >          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 += gen_endif(var.ifcond.cgen())
> >
> >      ret += mcgen('''
> >      } u;
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 67721b2470..56ea516399 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -79,7 +79,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 += gen_if(memb.ifcond.cgen())
> >          if memb.optional:
> >              ret += mcgen('''
> >      if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
> > @@ -112,7 +112,7 @@ def gen_visit_object_members(name: str,
> >              ret += mcgen('''
> >      }
> >  ''')
> > -        ret += gen_endif(memb.ifcond.ifcond)
> > +        ret += gen_endif(memb.ifcond.cgen())
> >
> >      if variants:
> >          tag_member = variants.tag_member
> > @@ -126,7 +126,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 += gen_if(var.ifcond.cgen())
> >              if var.type.name == 'q_empty':
> >                  # valid variant and nothing to do
> >                  ret += mcgen('''
> > @@ -142,7 +142,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 += gen_endif(var.ifcond.cgen())
> >          ret += mcgen('''
> >      default:
> >          abort();
> > @@ -228,7 +228,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 += gen_if(var.ifcond.cgen())
> >          ret += mcgen('''
> >      case %(case)s:
> >  ''',
> > @@ -254,7 +254,7 @@ def gen_visit_alternate(name: str, variants:
> QAPISchemaVariants) -> str:
> >          ret += mcgen('''
> >          break;
> >  ''')
> > -        ret += gen_endif(var.ifcond.ifcond)
> > +        ret += gen_endif(var.ifcond.cgen())
> >
> >      ret += mcgen('''
> >      case QTYPE_NONE:
>
>