Wrap generated enum/struct members and code with #if/#endif, using the
.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).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 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)
 
     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)
 
     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)
 
     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:
-- 
2.18.0.rc1

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Wrap generated enum/struct members and code with #if/#endif, using the

enum and struct members

> .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?

>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  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:

Hi
On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
> > ---
> >  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

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
> On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> 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).

I know...

> 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.

I'm not looking for reasons to split this patch, I'm looking for
stronger reasons to keep it just like it is :)

Your hunch that complications would arise for simple unions plausible:
there the same conditional needs to be applied both to the C enum's
member and the C union member.

For the generated C code to compile, each union tag enum member
conditional must imply the corresponding variant conditional.

For flat unions, the two are separate.  The QAPI generator makes no
effort to check the enum member's if condition implies the union
variant's if condition; if you mess them up in the schema, you get to
deal with the C compilation errors.

For simple unions, the two are one.

If we separate the generator updates for enums and for union members,
and do enum members first, then unions with conditional tag members
can't compile.  Corrollary: simple unions with conditional variants
can't compile.

What if we do union members first?

Again, I'm not asking for patch splitting here, I'm just trying to
arrive at a clearer understanding to avoid making insufficiently
supported claims in the commit message.  The combined patch looks small
and clean enough to keep it combined.

[...]

Markus Armbruster <armbru@redhat.com> writes:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Hi
>> On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Marc-André Lureau <marcandre.lureau@redhat.com> 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).
>
> I know...
>
>> 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.
>
> I'm not looking for reasons to split this patch, I'm looking for
> stronger reasons to keep it just like it is :)
>
> Your hunch that complications would arise for simple unions plausible:
> there the same conditional needs to be applied both to the C enum's
> member and the C union member.
>
> For the generated C code to compile, each union tag enum member
> conditional must imply the corresponding variant conditional.
>
> For flat unions, the two are separate.  The QAPI generator makes no
> effort to check the enum member's if condition implies the union
> variant's if condition; if you mess them up in the schema, you get to
> deal with the C compilation errors.
>
> For simple unions, the two are one.
>
> If we separate the generator updates for enums and for union members,
> and do enum members first, then unions with conditional tag members
> can't compile.  Corrollary: simple unions with conditional variants
> can't compile.
>
> What if we do union members first?
>
> Again, I'm not asking for patch splitting here, I'm just trying to
> arrive at a clearer understanding to avoid making insufficiently
> supported claims in the commit message.  The combined patch looks small
> and clean enough to keep it combined.
>
> [...]

What about this commit message:

    qapi: Add #if conditions to generated code members

    Wrap generated enum and struct members and their supporting code with
    #if/#endif, using the .ifcond members added in the previous patches.

    We do enum and struct in a single patch because union tag enum and the
    associated variants tie them together, and dealing with that to split
    the patch doesn't seem worthwhile.

On Mon, Dec 10, 2018 at 2:11 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Markus Armbruster <armbru@redhat.com> writes:
>
> > Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> >
> >> Hi
> >> On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>>
> >>> Marc-André Lureau <marcandre.lureau@redhat.com> 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).
> >
> > I know...
> >
> >> 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.
> >
> > I'm not looking for reasons to split this patch, I'm looking for
> > stronger reasons to keep it just like it is :)
> >
> > Your hunch that complications would arise for simple unions plausible:
> > there the same conditional needs to be applied both to the C enum's
> > member and the C union member.
> >
> > For the generated C code to compile, each union tag enum member
> > conditional must imply the corresponding variant conditional.
> >
> > For flat unions, the two are separate.  The QAPI generator makes no
> > effort to check the enum member's if condition implies the union
> > variant's if condition; if you mess them up in the schema, you get to
> > deal with the C compilation errors.
> >
> > For simple unions, the two are one.
> >
> > If we separate the generator updates for enums and for union members,
> > and do enum members first, then unions with conditional tag members
> > can't compile.  Corrollary: simple unions with conditional variants
> > can't compile.
> >
> > What if we do union members first?
> >
> > Again, I'm not asking for patch splitting here, I'm just trying to
> > arrive at a clearer understanding to avoid making insufficiently
> > supported claims in the commit message.  The combined patch looks small
> > and clean enough to keep it combined.
> >
> > [...]
>
> What about this commit message:
>
>     qapi: Add #if conditions to generated code members
>
>     Wrap generated enum and struct members and their supporting code with
>     #if/#endif, using the .ifcond members added in the previous patches.
>
>     We do enum and struct in a single patch because union tag enum and the
>     associated variants tie them together, and dealing with that to split
>     the patch doesn't seem worthwhile.
>


ack, thanks

-- 
Marc-André Lureau

On Mon, Dec 10, 2018 at 2:11 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Markus Armbruster <armbru@redhat.com> writes:
>
> > Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> >
> >> Hi
> >> On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>>
> >>> Marc-André Lureau <marcandre.lureau@redhat.com> 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).
> >
> > I know...
> >
> >> 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.
> >
> > I'm not looking for reasons to split this patch, I'm looking for
> > stronger reasons to keep it just like it is :)
> >
> > Your hunch that complications would arise for simple unions plausible:
> > there the same conditional needs to be applied both to the C enum's
> > member and the C union member.
> >
> > For the generated C code to compile, each union tag enum member
> > conditional must imply the corresponding variant conditional.
> >
> > For flat unions, the two are separate.  The QAPI generator makes no
> > effort to check the enum member's if condition implies the union
> > variant's if condition; if you mess them up in the schema, you get to
> > deal with the C compilation errors.
> >
> > For simple unions, the two are one.
> >
> > If we separate the generator updates for enums and for union members,
> > and do enum members first, then unions with conditional tag members
> > can't compile.  Corrollary: simple unions with conditional variants
> > can't compile.
> >
> > What if we do union members first?
> >
> > Again, I'm not asking for patch splitting here, I'm just trying to
> > arrive at a clearer understanding to avoid making insufficiently
> > supported claims in the commit message.  The combined patch looks small
> > and clean enough to keep it combined.
> >
> > [...]
>
> What about this commit message:
>
>     qapi: Add #if conditions to generated code members
>
>     Wrap generated enum and struct members and their supporting code with
>     #if/#endif, using the .ifcond members added in the previous patches.
>
>     We do enum and struct in a single patch because union tag enum and the
>     associated variants tie them together, and dealing with that to split
>     the patch doesn't seem worthwhile.

ack, thanks