Re: [Qemu-devel] [PATCH v6 21/27] qapi: add #if conditions to generated code members

[Marc-André Lureau]

Hi
On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster <address@hidden> wrote:
>
> Marc-André Lureau <address@hidden> writes:
>
> > Wrap generated enum/struct members and code with #if/#endif, using the
>
> enum and struct members

ok

>
> > .ifcond members added in the previous patches.
> >
> > Some types generate both enum and struct members for example, so a
> > step-by-step is unnecessarily complicated to deal with (it would
> > easily generate invalid intermediary code).
>
> Can you give an example of a schema definition that would lead to
> complications?
>

Honestly, I don't remember well (it's been a while I wrote that code).
It must be related to implicit enums, such as union kind... If there
is no strong need to split this patch, I would rather not do that
extra work.

> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  scripts/qapi/common.py     |  4 ++++
> >  scripts/qapi/introspect.py | 13 +++++++++----
> >  scripts/qapi/types.py      |  4 ++++
> >  scripts/qapi/visit.py      |  6 ++++++
> >  4 files changed, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index b3b64a60bf..a66c035b91 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -2098,11 +2098,13 @@ const QEnumLookup %(c_name)s_lookup = {
> >  ''',
> >                  c_name=c_name(name))
> >      for m in members:
> > +        ret += gen_if(m.ifcond)
> >          index = c_enum_const(name, m.name, prefix)
> >          ret += mcgen('''
> >          [%(index)s] = "%(name)s",
> >  ''',
> >                       index=index, name=m.name)
> > +        ret += gen_endif(m.ifcond)
> >
> >      ret += mcgen('''
> >      },
> > @@ -2124,10 +2126,12 @@ typedef enum %(c_name)s {
> >                  c_name=c_name(name))
> >
> >      for m in enum_members:
> > +        ret += gen_if(m.ifcond)
> >          ret += mcgen('''
> >      %(c_enum)s,
> >  ''',
> >                       c_enum=c_enum_const(name, m.name, prefix))
> > +        ret += gen_endif(m.ifcond)
> >
> >      ret += mcgen('''
> >  } %(c_name)s;
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index 3f1ca99f6d..bf5db51f4a 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -148,6 +148,8 @@ const QLitObject %(c_name)s = %(c_string)s;
> >          ret = {'name': member.name, 'type': self._use_type(member.type)}
> >          if member.optional:
> >              ret['default'] = None
> > +        if member.ifcond:
> > +            ret = (ret, member.ifcond)
> >          return ret
> >
> >      def _gen_variants(self, tag_name, variants):
> > @@ -155,14 +157,16 @@ const QLitObject %(c_name)s = %(c_string)s;
> >                  'variants': [self._gen_variant(v) for v in variants]}
> >
> >      def _gen_variant(self, variant):
> > -        return {'case': variant.name, 'type': self._use_type(variant.type)}
> > +        return ({'case': variant.name, 'type': 
> > self._use_type(variant.type)},
> > +                variant.ifcond)
>
> Looks different in your rebased version at
> https://github.com/elmarco/qemu.git branch qapi-if.  I'm only skimming
> this version.
>
> Note to self: always creates the tuple form, even when there's no
> if-condition.  Differs from _gen_qlit(), which creates a tuple only when
> there's a if-condition.  Not wrong, just a bit inconsistent.
>
> >
> >      def visit_builtin_type(self, name, info, json_type):
> >          self._gen_qlit(name, 'builtin', {'json-type': json_type}, [])
> >
> >      def visit_enum_type(self, name, info, ifcond, members, prefix):
> >          self._gen_qlit(name, 'enum',
> > -                       {'values': [m.name for m in members]}, ifcond)
> > +                       {'values': [(m.name, m.ifcond) for m in members]},
> > +                       ifcond)
>
> Likewise.
>
> >
> >      def visit_array_type(self, name, info, ifcond, element_type):
> >          element = self._use_type(element_type)
> > @@ -178,8 +182,9 @@ const QLitObject %(c_name)s = %(c_string)s;
> >
> >      def visit_alternate_type(self, name, info, ifcond, variants):
> >          self._gen_qlit(name, 'alternate',
> > -                       {'members': [{'type': self._use_type(m.type)}
> > -                                    for m in variants.variants]}, ifcond)
> > +                       {'members': [
> > +                           ({'type': self._use_type(m.type)}, m.ifcond)
> > +                           for m in variants.variants]}, ifcond)
>
> Likewise.
>
> >
> >      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> >                        success_response, boxed, allow_oob, allow_preconfig):
> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> > index 2d4a70f810..7d9eef6320 100644
> > --- a/scripts/qapi/types.py
> > +++ b/scripts/qapi/types.py
> > @@ -43,6 +43,7 @@ struct %(c_name)s {
> >  def gen_struct_members(members):
> >      ret = ''
> >      for memb in members:
> > +        ret += gen_if(memb.ifcond)
> >          if memb.optional:
> >              ret += mcgen('''
> >      bool has_%(c_name)s;
> > @@ -52,6 +53,7 @@ def gen_struct_members(members):
> >      %(c_type)s %(c_name)s;
> >  ''',
> >                       c_type=memb.type.c_type(), c_name=c_name(memb.name))
> > +        ret += gen_endif(memb.ifcond)
> >      return ret
> >
> >
> > @@ -131,11 +133,13 @@ def gen_variants(variants):
> >      for var in variants.variants:
> >          if var.type.name == 'q_empty':
> >              continue
> > +        ret += gen_if(var.ifcond)
> >          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)
> >
> >      ret += mcgen('''
> >      } u;
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 24f85a2e85..82eab72b21 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -54,6 +54,7 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
> > *obj, Error **errp)
> >                       c_type=base.c_name())
> >
> >      for memb in members:
> > +        ret += gen_if(memb.ifcond)
> >          if memb.optional:
> >              ret += mcgen('''
> >      if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
> > @@ -73,6 +74,7 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
> > *obj, Error **errp)
> >              ret += mcgen('''
> >      }
> >  ''')
> > +        ret += gen_endif(memb.ifcond)
> >
> >      if variants:
> >          ret += mcgen('''
> > @@ -84,6 +86,7 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
> > *obj, Error **errp)
> >              case_str = c_enum_const(variants.tag_member.type.name,
> >                                      var.name,
> >                                      variants.tag_member.type.prefix)
> > +            ret += gen_if(var.ifcond)
> >              if var.type.name == 'q_empty':
> >                  # valid variant and nothing to do
> >                  ret += mcgen('''
> > @@ -100,6 +103,7 @@ void visit_type_%(c_name)s_members(Visitor *v, 
> > %(c_name)s *obj, Error **errp)
> >                               case=case_str,
> >                               c_type=var.type.c_name(), 
> > c_name=c_name(var.name))
> >
> > +            ret += gen_endif(var.ifcond)
> >          ret += mcgen('''
> >      default:
> >          abort();
> > @@ -190,6 +194,7 @@ void visit_type_%(c_name)s(Visitor *v, const char 
> > *name, %(c_name)s **obj, Error
> >                  c_name=c_name(name))
> >
> >      for var in variants.variants:
> > +        ret += gen_if(var.ifcond)
> >          ret += mcgen('''
> >      case %(case)s:
> >  ''',
> > @@ -217,6 +222,7 @@ void visit_type_%(c_name)s(Visitor *v, const char 
> > *name, %(c_name)s **obj, Error
> >          ret += mcgen('''
> >          break;
> >  ''')
> > +        ret += gen_endif(var.ifcond)
> >
> >      ret += mcgen('''
> >      case QTYPE_NONE:

thanks