[v2,5/6] qapi: generate list struct and visit_list for enum

Currently, if define an 'enum' and use it in one command's data,
List struct for enum could not be generated, but it's used in
qmp function.

For example: KeyCodesList could not be generated.
>>> qapi-schema.json:
{ 'enum': 'KeyCodes',
  'data': [ 'shift', 'alt' ... ] }
{ 'command': 'sendkey',
  'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }

>>> qmp-command.h:
void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
hold_time, Error **errp);

This patch makes qapi can generate List struct for enum.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
 scripts/qapi-visit.py |   14 +++++++++-----
 2 files changed, 38 insertions(+), 9 deletions(-)

----- Original Message -----
> Currently, if define an 'enum' and use it in one command's data,
> List struct for enum could not be generated, but it's used in
> qmp function.
> 
> For example: KeyCodesList could not be generated.
> >>> qapi-schema.json:
> { 'enum': 'KeyCodes',
>   'data': [ 'shift', 'alt' ... ] }
> { 'command': 'sendkey',
>   'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> 
> >>> qmp-command.h:
> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
> hold_time, Error **errp);
> 
> This patch makes qapi can generate List struct for enum.


Anthony, Mike, any comments on this patch?


> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
>  scripts/qapi-visit.py |   14 +++++++++-----
>  2 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4a734f5..c9641fb 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -16,17 +16,36 @@ import os
>  import getopt
>  import errno
>  
> -def generate_fwd_struct(name, members):
> -    return mcgen('''
> +def generate_fwd_struct(name, members, enum=False):
> +    ret = ""
> +    if not enum:
> +        ret += mcgen('''
>  typedef struct %(name)s %(name)s;
>  
> +''',
> +                     name=name)
> +    ret += mcgen('''
>  typedef struct %(name)sList
>  {
> -    %(name)s *value;
> +''',
> +                     name=name)
> +    if enum:
> +        ret += mcgen('''
> +         %(name)s value;
> +''',
> +                     name=name)
> +    else:
> +        ret += mcgen('''
> +         %(name)s * value;
> +''',
> +                     name=name)
> +
> +    ret += mcgen('''
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
>                   name=name)
> +    return ret
>  
>  def generate_struct(structname, fieldname, members):
>      ret = mcgen('''
> @@ -265,7 +284,8 @@ for expr in exprs:
>      if expr.has_key('type'):
>          ret += generate_fwd_struct(expr['type'], expr['data'])
>      elif expr.has_key('enum'):
> -        ret += generate_enum(expr['enum'], expr['data'])
> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
>          fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>      elif expr.has_key('union'):
>          ret += generate_fwd_struct(expr['union'], expr['data']) +
>          "\n"
> @@ -289,6 +309,11 @@ for expr in exprs:
>          fdef.write(generate_type_cleanup(expr['union'] + "List") +
>          "\n")
>          ret += generate_type_cleanup_decl(expr['union'])
>          fdef.write(generate_type_cleanup(expr['union']) + "\n")
> +    elif expr.has_key('enum'):
> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") +
> "\n")
> +        ret += generate_type_cleanup_decl(expr['enum'])
> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
>      else:
>          continue
>      fdecl.write(ret)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 8d4e94a..e44edfa 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -81,7 +81,7 @@ end:
>  ''')
>      return ret
>  
> -def generate_visit_list(name, members):
> +def generate_visit_list(name, members, enum=False):
>      return mcgen('''
>  
>  void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const
>  char *name, Error **errp)
> @@ -160,12 +160,14 @@ end:
>  
>      return ret
>  
> -def generate_declaration(name, members, genlist=True):
> -    ret = mcgen('''
> +def generate_declaration(name, members, genlist=True, enum=False):
> +    ret = ""
> +    if not enum:
> +        ret = mcgen('''
>  
>  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char
>  *name, Error **errp);
>  ''',
> -                name=name)
> +                    name=name)
>  
>      if genlist:
>          ret += mcgen('''
> @@ -293,10 +295,12 @@ for expr in exprs:
>          ret += generate_declaration(expr['union'], expr['data'])
>          fdecl.write(ret)
>      elif expr.has_key('enum'):
> -        ret = generate_visit_enum(expr['enum'], expr['data'])
> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
> +        ret += generate_visit_enum(expr['enum'], expr['data'])
>          fdef.write(ret)
>  
>          ret = generate_decl_enum(expr['enum'], expr['data'])
> +        ret += generate_declaration(expr['enum'], expr['data'],
> enum=True)
>          fdecl.write(ret)
>  
>  fdecl.write('''
> --
> 1.7.1
> 
> 
>

On Sat,  2 Jun 2012 06:54:27 +0800
Amos Kong <akong@redhat.com> wrote:

> Currently, if define an 'enum' and use it in one command's data,
> List struct for enum could not be generated, but it's used in
> qmp function.
> 
> For example: KeyCodesList could not be generated.
> >>> qapi-schema.json:
> { 'enum': 'KeyCodes',
>   'data': [ 'shift', 'alt' ... ] }
> { 'command': 'sendkey',
>   'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> 
> >>> qmp-command.h:
> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
> hold_time, Error **errp);
> 
> This patch makes qapi can generate List struct for enum.

This patch does it the simple way, just like any type. It generates a enum list
type and the functions qapi_free_yourenum() and qapi_free_yourenumlist().

The qapi_free_yourenum() list ends up doing nothing, so it could be a good
idea to generate an empty body (also note that we're copying the argument's
value, this could bite us in the future).

Another point I was wondering is that, all enums will end up having the
exact same code. So maybe we could generate a default int list and use it
for all enums. Not sure it's worth it though.

Michael, Paolo, ideas?

More review comments below.


> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
>  scripts/qapi-visit.py |   14 +++++++++-----
>  2 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4a734f5..c9641fb 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -16,17 +16,36 @@ import os
>  import getopt
>  import errno
>  
> -def generate_fwd_struct(name, members):
> -    return mcgen('''
> +def generate_fwd_struct(name, members, enum=False):

I think it's better to have generate_fwd_enum_struct().

> +    ret = ""
> +    if not enum:
> +        ret += mcgen('''
>  typedef struct %(name)s %(name)s;
>  
> +''',
> +                     name=name)
> +    ret += mcgen('''
>  typedef struct %(name)sList
>  {
> -    %(name)s *value;
> +''',
> +                     name=name)
> +    if enum:
> +        ret += mcgen('''
> +         %(name)s value;
> +''',
> +                     name=name)
> +    else:
> +        ret += mcgen('''
> +         %(name)s * value;
> +''',
> +                     name=name)
> +
> +    ret += mcgen('''
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
>                   name=name)
> +    return ret
>  
>  def generate_struct(structname, fieldname, members):
>      ret = mcgen('''
> @@ -265,7 +284,8 @@ for expr in exprs:
>      if expr.has_key('type'):
>          ret += generate_fwd_struct(expr['type'], expr['data'])
>      elif expr.has_key('enum'):
> -        ret += generate_enum(expr['enum'], expr['data'])
> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"

The new-line should be returned by generate_enum(). Same applies for the
occurrences below.

> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
>          fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>      elif expr.has_key('union'):
>          ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
> @@ -289,6 +309,11 @@ for expr in exprs:
>          fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
>          ret += generate_type_cleanup_decl(expr['union'])
>          fdef.write(generate_type_cleanup(expr['union']) + "\n")
> +    elif expr.has_key('enum'):
> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
> +        ret += generate_type_cleanup_decl(expr['enum'])
> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
>      else:
>          continue
>      fdecl.write(ret)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 8d4e94a..e44edfa 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -81,7 +81,7 @@ end:
>  ''')
>      return ret
>  
> -def generate_visit_list(name, members):
> +def generate_visit_list(name, members, enum=False):
>      return mcgen('''
>  
>  void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
> @@ -160,12 +160,14 @@ end:
>  
>      return ret
>  
> -def generate_declaration(name, members, genlist=True):
> -    ret = mcgen('''
> +def generate_declaration(name, members, genlist=True, enum=False):
> +    ret = ""
> +    if not enum:
> +        ret = mcgen('''
>  
>  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
>  ''',
> -                name=name)
> +                    name=name)

Why this change?

>  
>      if genlist:
>          ret += mcgen('''
> @@ -293,10 +295,12 @@ for expr in exprs:
>          ret += generate_declaration(expr['union'], expr['data'])
>          fdecl.write(ret)
>      elif expr.has_key('enum'):
> -        ret = generate_visit_enum(expr['enum'], expr['data'])
> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
> +        ret += generate_visit_enum(expr['enum'], expr['data'])
>          fdef.write(ret)
>  
>          ret = generate_decl_enum(expr['enum'], expr['data'])
> +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
>          fdecl.write(ret)
>  
>  fdecl.write('''

On Sat, Jun 02, 2012 at 06:54:27AM +0800, Amos Kong wrote:
> Currently, if define an 'enum' and use it in one command's data,
> List struct for enum could not be generated, but it's used in
> qmp function.
> 
> For example: KeyCodesList could not be generated.
> >>> qapi-schema.json:
> { 'enum': 'KeyCodes',
>   'data': [ 'shift', 'alt' ... ] }
> { 'command': 'sendkey',
>   'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> 
> >>> qmp-command.h:
> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
> hold_time, Error **errp);
> 
> This patch makes qapi can generate List struct for enum.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
>  scripts/qapi-visit.py |   14 +++++++++-----
>  2 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4a734f5..c9641fb 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -16,17 +16,36 @@ import os
>  import getopt
>  import errno
> 
> -def generate_fwd_struct(name, members):
> -    return mcgen('''
> +def generate_fwd_struct(name, members, enum=False):
> +    ret = ""
> +    if not enum:
> +        ret += mcgen('''
>  typedef struct %(name)s %(name)s;
> 
> +''',
> +                     name=name)
> +    ret += mcgen('''
>  typedef struct %(name)sList
>  {
> -    %(name)s *value;
> +''',
> +                     name=name)
> +    if enum:
> +        ret += mcgen('''
> +         %(name)s value;
> +''',
> +                     name=name)
> +    else:
> +        ret += mcgen('''
> +         %(name)s * value;
> +''',
> +                     name=name)
> +
> +    ret += mcgen('''
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
>                   name=name)
> +    return ret
> 
>  def generate_struct(structname, fieldname, members):
>      ret = mcgen('''
> @@ -265,7 +284,8 @@ for expr in exprs:
>      if expr.has_key('type'):
>          ret += generate_fwd_struct(expr['type'], expr['data'])
>      elif expr.has_key('enum'):
> -        ret += generate_enum(expr['enum'], expr['data'])
> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
>          fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>      elif expr.has_key('union'):
>          ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
> @@ -289,6 +309,11 @@ for expr in exprs:
>          fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
>          ret += generate_type_cleanup_decl(expr['union'])
>          fdef.write(generate_type_cleanup(expr['union']) + "\n")
> +    elif expr.has_key('enum'):
> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")

This one is fine

> +        ret += generate_type_cleanup_decl(expr['enum'])
> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")

but we don't need this one since enum types are passed around by copy.

The qapi_free_X() functions are utility functions that aren't used by
the code generators, so there should be any harm in omitting these.


>      else:
>          continue
>      fdecl.write(ret)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 8d4e94a..e44edfa 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -81,7 +81,7 @@ end:
>  ''')
>      return ret
> 
> -def generate_visit_list(name, members):
> +def generate_visit_list(name, members, enum=False):
>      return mcgen('''
> 

Were there plans to add a branch for enum=True? Otherwise we can drop this
from the patch.

>  void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
> @@ -160,12 +160,14 @@ end:
> 
>      return ret
> 
> -def generate_declaration(name, members, genlist=True):
> -    ret = mcgen('''
> +def generate_declaration(name, members, genlist=True, enum=False):
> +    ret = ""
> +    if not enum:
> +        ret = mcgen('''
> 
>  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
>  ''',
> -                name=name)
> +                    name=name)
> 
>      if genlist:
>          ret += mcgen('''
> @@ -293,10 +295,12 @@ for expr in exprs:
>          ret += generate_declaration(expr['union'], expr['data'])
>          fdecl.write(ret)
>      elif expr.has_key('enum'):
> -        ret = generate_visit_enum(expr['enum'], expr['data'])
> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
> +        ret += generate_visit_enum(expr['enum'], expr['data'])
>          fdef.write(ret)
> 
>          ret = generate_decl_enum(expr['enum'], expr['data'])
> +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
>          fdecl.write(ret)
> 
>  fdecl.write('''
> -- 
> 1.7.1
> 
>

On Wed, Jun 06, 2012 at 03:40:37PM -0300, Luiz Capitulino wrote:
> On Sat,  2 Jun 2012 06:54:27 +0800
> Amos Kong <akong@redhat.com> wrote:
> 
> > Currently, if define an 'enum' and use it in one command's data,
> > List struct for enum could not be generated, but it's used in
> > qmp function.
> > 
> > For example: KeyCodesList could not be generated.
> > >>> qapi-schema.json:
> > { 'enum': 'KeyCodes',
> >   'data': [ 'shift', 'alt' ... ] }
> > { 'command': 'sendkey',
> >   'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> > 
> > >>> qmp-command.h:
> > void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
> > hold_time, Error **errp);
> > 
> > This patch makes qapi can generate List struct for enum.
> 
> This patch does it the simple way, just like any type. It generates a enum list
> type and the functions qapi_free_yourenum() and qapi_free_yourenumlist().

Had a couple suggestions, but approach/patch seems reasonable to me.

> 
> The qapi_free_yourenum() list ends up doing nothing, so it could be a good
> idea to generate an empty body (also note that we're copying the argument's
> value, this could bite us in the future).

I think we can omit qapi_free_yourenum() completely. Humans will know
not to free non-allocated types, and the generated marshallers
don't use these interfaces.

> 
> Another point I was wondering is that, all enums will end up having the
> exact same code. So maybe we could generate a default int list and use it
> for all enums. Not sure it's worth it though.

Since it's generated code I don't think it's worth it, personally.

> 
> Michael, Paolo, ideas?
> 
> More review comments below.
> 
> 
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
> >  scripts/qapi-visit.py |   14 +++++++++-----
> >  2 files changed, 38 insertions(+), 9 deletions(-)
> > 
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index 4a734f5..c9641fb 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -16,17 +16,36 @@ import os
> >  import getopt
> >  import errno
> >  
> > -def generate_fwd_struct(name, members):
> > -    return mcgen('''
> > +def generate_fwd_struct(name, members, enum=False):
> 
> I think it's better to have generate_fwd_enum_struct().
> 
> > +    ret = ""
> > +    if not enum:
> > +        ret += mcgen('''
> >  typedef struct %(name)s %(name)s;
> >  
> > +''',
> > +                     name=name)
> > +    ret += mcgen('''
> >  typedef struct %(name)sList
> >  {
> > -    %(name)s *value;
> > +''',
> > +                     name=name)
> > +    if enum:
> > +        ret += mcgen('''
> > +         %(name)s value;
> > +''',
> > +                     name=name)
> > +    else:
> > +        ret += mcgen('''
> > +         %(name)s * value;
> > +''',
> > +                     name=name)
> > +
> > +    ret += mcgen('''
> >      struct %(name)sList *next;
> >  } %(name)sList;
> >  ''',
> >                   name=name)
> > +    return ret
> >  
> >  def generate_struct(structname, fieldname, members):
> >      ret = mcgen('''
> > @@ -265,7 +284,8 @@ for expr in exprs:
> >      if expr.has_key('type'):
> >          ret += generate_fwd_struct(expr['type'], expr['data'])
> >      elif expr.has_key('enum'):
> > -        ret += generate_enum(expr['enum'], expr['data'])
> > +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
> 
> The new-line should be returned by generate_enum(). Same applies for the
> occurrences below.
> 
> > +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
> >          fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
> >      elif expr.has_key('union'):
> >          ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
> > @@ -289,6 +309,11 @@ for expr in exprs:
> >          fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
> >          ret += generate_type_cleanup_decl(expr['union'])
> >          fdef.write(generate_type_cleanup(expr['union']) + "\n")
> > +    elif expr.has_key('enum'):
> > +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
> > +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
> > +        ret += generate_type_cleanup_decl(expr['enum'])
> > +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
> >      else:
> >          continue
> >      fdecl.write(ret)
> > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> > index 8d4e94a..e44edfa 100644
> > --- a/scripts/qapi-visit.py
> > +++ b/scripts/qapi-visit.py
> > @@ -81,7 +81,7 @@ end:
> >  ''')
> >      return ret
> >  
> > -def generate_visit_list(name, members):
> > +def generate_visit_list(name, members, enum=False):
> >      return mcgen('''
> >  
> >  void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
> > @@ -160,12 +160,14 @@ end:
> >  
> >      return ret
> >  
> > -def generate_declaration(name, members, genlist=True):
> > -    ret = mcgen('''
> > +def generate_declaration(name, members, genlist=True, enum=False):
> > +    ret = ""
> > +    if not enum:
> > +        ret = mcgen('''
> >  
> >  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
> >  ''',
> > -                name=name)
> > +                    name=name)
> 
> Why this change?
> 
> >  
> >      if genlist:
> >          ret += mcgen('''
> > @@ -293,10 +295,12 @@ for expr in exprs:
> >          ret += generate_declaration(expr['union'], expr['data'])
> >          fdecl.write(ret)
> >      elif expr.has_key('enum'):
> > -        ret = generate_visit_enum(expr['enum'], expr['data'])
> > +        ret = generate_visit_list(expr['enum'], expr['data'], True)
> > +        ret += generate_visit_enum(expr['enum'], expr['data'])
> >          fdef.write(ret)
> >  
> >          ret = generate_decl_enum(expr['enum'], expr['data'])
> > +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
> >          fdecl.write(ret)
> >  
> >  fdecl.write('''
> 
>

On 07/06/12 08:26, Michael Roth wrote:
> On Wed, Jun 06, 2012 at 03:40:37PM -0300, Luiz Capitulino wrote:
>> On Sat,  2 Jun 2012 06:54:27 +0800
>> Amos Kong<akong@redhat.com>  wrote:
>>
>>> Currently, if define an 'enum' and use it in one command's data,
>>> List struct for enum could not be generated, but it's used in
>>> qmp function.
>>>
>>> For example: KeyCodesList could not be generated.
>>>>>> qapi-schema.json:
>>> { 'enum': 'KeyCodes',
>>>    'data': [ 'shift', 'alt' ... ] }
>>> { 'command': 'sendkey',
>>>    'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
>>>
>>>>>> qmp-command.h:
>>> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
>>> hold_time, Error **errp);
>>>
>>> This patch makes qapi can generate List struct for enum.
>>
>> This patch does it the simple way, just like any type. It generates a enum list
>> type and the functions qapi_free_yourenum() and qapi_free_yourenumlist().
>
> Had a couple suggestions, but approach/patch seems reasonable to me.
>
>>
>> The qapi_free_yourenum() list ends up doing nothing, so it could be a good
>> idea to generate an empty body (also note that we're copying the argument's
>> value, this could bite us in the future).
>
> I think we can omit qapi_free_yourenum() completely. Humans will know
> not to free non-allocated types, and the generated marshallers
> don't use these interfaces.
>
>>
>> Another point I was wondering is that, all enums will end up having the
>> exact same code. So maybe we could generate a default int list and use it
>> for all enums. Not sure it's worth it though.
>
> Since it's generated code I don't think it's worth it, personally.
>
>>
>> Michael, Paolo, ideas?
>>
>> More review comments below.


Thanks for your review!

>>> Signed-off-by: Amos Kong<akong@redhat.com>
>>> ---
>>>   scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
>>>   scripts/qapi-visit.py |   14 +++++++++-----
>>>   2 files changed, 38 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>>> index 4a734f5..c9641fb 100644
>>> --- a/scripts/qapi-types.py
>>> +++ b/scripts/qapi-types.py
>>> @@ -16,17 +16,36 @@ import os
>>>   import getopt
>>>   import errno
>>>
>>> -def generate_fwd_struct(name, members):
>>> -    return mcgen('''
>>> +def generate_fwd_struct(name, members, enum=False):
>>
>> I think it's better to have generate_fwd_enum_struct().

I tried this before, too many duplicate codes would be introduced,
but it's more clear.

two functions need to be added
   qapi-types.py: def generate_fwd_enum_struct(name, members):
   qapi-visit.py: def generate_enum_declaration(name, members, 
genlist=True):

>>> +    ret = ""
>>> +    if not enum:
>>> +        ret += mcgen('''
>>>   typedef struct %(name)s %(name)s;
>>>
>>> +''',
>>> +                     name=name)
>>> +    ret += mcgen('''
>>>   typedef struct %(name)sList
>>>   {
>>> -    %(name)s *value;
>>> +''',
>>> +                     name=name)
>>> +    if enum:
>>> +        ret += mcgen('''
>>> +         %(name)s value;
>>> +''',
>>> +                     name=name)
>>> +    else:
>>> +        ret += mcgen('''
>>> +         %(name)s * value;
>>> +''',
>>> +                     name=name)
>>> +
>>> +    ret += mcgen('''
>>>       struct %(name)sList *next;
>>>   } %(name)sList;
>>>   ''',
>>>                    name=name)
>>> +    return ret
>>>
>>>   def generate_struct(structname, fieldname, members):
>>>       ret = mcgen('''
>>> @@ -265,7 +284,8 @@ for expr in exprs:
>>>       if expr.has_key('type'):
>>>           ret += generate_fwd_struct(expr['type'], expr['data'])
>>>       elif expr.has_key('enum'):
>>> -        ret += generate_enum(expr['enum'], expr['data'])
>>> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
>>
>> The new-line should be returned by generate_enum(). Same applies for the
>> occurrences below.


"\n" is used to add a blank line here.

>>> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
>>>           fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>>>       elif expr.has_key('union'):
>>>           ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
>>> @@ -289,6 +309,11 @@ for expr in exprs:
>>>           fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
>>>           ret += generate_type_cleanup_decl(expr['union'])
>>>           fdef.write(generate_type_cleanup(expr['union']) + "\n")
>>> +    elif expr.has_key('enum'):
>>> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
>>> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
>>> +        ret += generate_type_cleanup_decl(expr['enum'])
>>> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
>>>       else:
>>>           continue
>>>       fdecl.write(ret)
>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>>> index 8d4e94a..e44edfa 100644
>>> --- a/scripts/qapi-visit.py
>>> +++ b/scripts/qapi-visit.py
>>> @@ -81,7 +81,7 @@ end:
>>>   ''')
>>>       return ret
>>>
>>> -def generate_visit_list(name, members):
>>> +def generate_visit_list(name, members, enum=False):
>>>       return mcgen('''
>>>
>>>   void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
>>> @@ -160,12 +160,14 @@ end:
>>>
>>>       return ret
>>>
>>> -def generate_declaration(name, members, genlist=True):
>>> -    ret = mcgen('''
>>> +def generate_declaration(name, members, genlist=True, enum=False):
>>> +    ret = ""
>>> +    if not enum:
>>> +        ret = mcgen('''

>>>   void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
>>>   ''',
>>> -                name=name)
>>> +                    name=name)
>>
>> Why this change?


Indent of above "mcgen(" changes, make them aligned.

>>>
>>>       if genlist:
>>>           ret += mcgen('''
>>> @@ -293,10 +295,12 @@ for expr in exprs:
>>>           ret += generate_declaration(expr['union'], expr['data'])
>>>           fdecl.write(ret)
>>>       elif expr.has_key('enum'):
>>> -        ret = generate_visit_enum(expr['enum'], expr['data'])
>>> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
>>> +        ret += generate_visit_enum(expr['enum'], expr['data'])
>>>           fdef.write(ret)
>>>
>>>           ret = generate_decl_enum(expr['enum'], expr['data'])
>>> +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
>>>           fdecl.write(ret)
>>>
>>>   fdecl.write('''
>>
>>

On 07/06/12 08:15, Michael Roth wrote:
> On Sat, Jun 02, 2012 at 06:54:27AM +0800, Amos Kong wrote:
>> Currently, if define an 'enum' and use it in one command's data,
>> List struct for enum could not be generated, but it's used in
>> qmp function.
>>
>> For example: KeyCodesList could not be generated.
>>>>> qapi-schema.json:
>> { 'enum': 'KeyCodes',
>>    'data': [ 'shift', 'alt' ... ] }
>> { 'command': 'sendkey',
>>    'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
>>
>>>>> qmp-command.h:
>> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
>> hold_time, Error **errp);
>>
>> This patch makes qapi can generate List struct for enum.
>>
>> Signed-off-by: Amos Kong<akong@redhat.com>
>> ---
>>   scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
>>   scripts/qapi-visit.py |   14 +++++++++-----
>>   2 files changed, 38 insertions(+), 9 deletions(-)
>>
>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>> index 4a734f5..c9641fb 100644
>> --- a/scripts/qapi-types.py
>> +++ b/scripts/qapi-types.py
>> @@ -16,17 +16,36 @@ import os
>>   import getopt
>>   import errno
>>
>> -def generate_fwd_struct(name, members):
>> -    return mcgen('''
>> +def generate_fwd_struct(name, members, enum=False):
>> +    ret = ""
>> +    if not enum:
>> +        ret += mcgen('''
>>   typedef struct %(name)s %(name)s;
>>
>> +''',
>> +                     name=name)
>> +    ret += mcgen('''
>>   typedef struct %(name)sList
>>   {
>> -    %(name)s *value;
>> +''',
>> +                     name=name)
>> +    if enum:
>> +        ret += mcgen('''
>> +         %(name)s value;
>> +''',
>> +                     name=name)
>> +    else:
>> +        ret += mcgen('''
>> +         %(name)s * value;
>> +''',
>> +                     name=name)
>> +
>> +    ret += mcgen('''
>>       struct %(name)sList *next;
>>   } %(name)sList;
>>   ''',
>>                    name=name)
>> +    return ret
>>
>>   def generate_struct(structname, fieldname, members):
>>       ret = mcgen('''
>> @@ -265,7 +284,8 @@ for expr in exprs:
>>       if expr.has_key('type'):
>>           ret += generate_fwd_struct(expr['type'], expr['data'])
>>       elif expr.has_key('enum'):
>> -        ret += generate_enum(expr['enum'], expr['data'])
>> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
>> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
>>           fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>>       elif expr.has_key('union'):
>>           ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
>> @@ -289,6 +309,11 @@ for expr in exprs:
>>           fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
>>           ret += generate_type_cleanup_decl(expr['union'])
>>           fdef.write(generate_type_cleanup(expr['union']) + "\n")
>> +    elif expr.has_key('enum'):
>> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
>> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
>
> This one is fine
>
>> +        ret += generate_type_cleanup_decl(expr['enum'])
>> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
>
> but we don't need this one since enum types are passed around by copy.
>
> The qapi_free_X() functions are utility functions that aren't used by
> the code generators, so there should be any harm in omitting these.

Ok, will drop those two sentences.


>>       else:
>>           continue
>>       fdecl.write(ret)
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index 8d4e94a..e44edfa 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -81,7 +81,7 @@ end:
>>   ''')
>>       return ret
>>
>> -def generate_visit_list(name, members):
>> +def generate_visit_list(name, members, enum=False):
>>       return mcgen('''
>>
>
> Were there plans to add a branch for enum=True? Otherwise we can drop this
> from the patch.


redundant, will drop it.

>>   void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
>> @@ -160,12 +160,14 @@ end:
>>
>>       return ret
>>
>> -def generate_declaration(name, members, genlist=True):
>> -    ret = mcgen('''
>> +def generate_declaration(name, members, genlist=True, enum=False):
>> +    ret = ""
>> +    if not enum:
>> +        ret = mcgen('''
>>
>>   void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
>>   ''',
>> -                name=name)
>> +                    name=name)
>>
>>       if genlist:
>>           ret += mcgen('''
>> @@ -293,10 +295,12 @@ for expr in exprs:
>>           ret += generate_declaration(expr['union'], expr['data'])
>>           fdecl.write(ret)
>>       elif expr.has_key('enum'):
>> -        ret = generate_visit_enum(expr['enum'], expr['data'])
>> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
>> +        ret += generate_visit_enum(expr['enum'], expr['data'])
>>           fdef.write(ret)
>>
>>           ret = generate_decl_enum(expr['enum'], expr['data'])
>> +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
>>           fdecl.write(ret)
>>
>>   fdecl.write('''
>> --
>> 1.7.1
>>
>>

On Thu, 07 Jun 2012 10:52:46 +0800
Amos Kong <akong@redhat.com> wrote:

> On 07/06/12 08:26, Michael Roth wrote:
> > On Wed, Jun 06, 2012 at 03:40:37PM -0300, Luiz Capitulino wrote:
> >> On Sat,  2 Jun 2012 06:54:27 +0800
> >> Amos Kong<akong@redhat.com>  wrote:
> >>
> >>> Currently, if define an 'enum' and use it in one command's data,
> >>> List struct for enum could not be generated, but it's used in
> >>> qmp function.
> >>>
> >>> For example: KeyCodesList could not be generated.
> >>>>>> qapi-schema.json:
> >>> { 'enum': 'KeyCodes',
> >>>    'data': [ 'shift', 'alt' ... ] }
> >>> { 'command': 'sendkey',
> >>>    'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> >>>
> >>>>>> qmp-command.h:
> >>> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
> >>> hold_time, Error **errp);
> >>>
> >>> This patch makes qapi can generate List struct for enum.
> >>
> >> This patch does it the simple way, just like any type. It generates a enum list
> >> type and the functions qapi_free_yourenum() and qapi_free_yourenumlist().
> >
> > Had a couple suggestions, but approach/patch seems reasonable to me.
> >
> >>
> >> The qapi_free_yourenum() list ends up doing nothing, so it could be a good
> >> idea to generate an empty body (also note that we're copying the argument's
> >> value, this could bite us in the future).
> >
> > I think we can omit qapi_free_yourenum() completely. Humans will know
> > not to free non-allocated types, and the generated marshallers
> > don't use these interfaces.
> >
> >>
> >> Another point I was wondering is that, all enums will end up having the
> >> exact same code. So maybe we could generate a default int list and use it
> >> for all enums. Not sure it's worth it though.
> >
> > Since it's generated code I don't think it's worth it, personally.
> >
> >>
> >> Michael, Paolo, ideas?
> >>
> >> More review comments below.
> 
> 
> Thanks for your review!
> 
> >>> Signed-off-by: Amos Kong<akong@redhat.com>
> >>> ---
> >>>   scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
> >>>   scripts/qapi-visit.py |   14 +++++++++-----
> >>>   2 files changed, 38 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> >>> index 4a734f5..c9641fb 100644
> >>> --- a/scripts/qapi-types.py
> >>> +++ b/scripts/qapi-types.py
> >>> @@ -16,17 +16,36 @@ import os
> >>>   import getopt
> >>>   import errno
> >>>
> >>> -def generate_fwd_struct(name, members):
> >>> -    return mcgen('''
> >>> +def generate_fwd_struct(name, members, enum=False):
> >>
> >> I think it's better to have generate_fwd_enum_struct().
> 
> I tried this before, too many duplicate codes would be introduced,
> but it's more clear.

You could try to move the duplicated code to a third function (or move
the specific bits to generate_fwd_enum_struct() and generate_enum_declaration()).

> 
> two functions need to be added
>    qapi-types.py: def generate_fwd_enum_struct(name, members):
>    qapi-visit.py: def generate_enum_declaration(name, members, 
> genlist=True):
> 
> >>> +    ret = ""
> >>> +    if not enum:
> >>> +        ret += mcgen('''
> >>>   typedef struct %(name)s %(name)s;
> >>>
> >>> +''',
> >>> +                     name=name)
> >>> +    ret += mcgen('''
> >>>   typedef struct %(name)sList
> >>>   {
> >>> -    %(name)s *value;
> >>> +''',
> >>> +                     name=name)
> >>> +    if enum:
> >>> +        ret += mcgen('''
> >>> +         %(name)s value;
> >>> +''',
> >>> +                     name=name)
> >>> +    else:
> >>> +        ret += mcgen('''
> >>> +         %(name)s * value;
> >>> +''',
> >>> +                     name=name)
> >>> +
> >>> +    ret += mcgen('''
> >>>       struct %(name)sList *next;
> >>>   } %(name)sList;
> >>>   ''',
> >>>                    name=name)
> >>> +    return ret
> >>>
> >>>   def generate_struct(structname, fieldname, members):
> >>>       ret = mcgen('''
> >>> @@ -265,7 +284,8 @@ for expr in exprs:
> >>>       if expr.has_key('type'):
> >>>           ret += generate_fwd_struct(expr['type'], expr['data'])
> >>>       elif expr.has_key('enum'):
> >>> -        ret += generate_enum(expr['enum'], expr['data'])
> >>> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
> >>
> >> The new-line should be returned by generate_enum(). Same applies for the
> >> occurrences below.
> 
> 
> "\n" is used to add a blank line here.
> 
> >>> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
> >>>           fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
> >>>       elif expr.has_key('union'):
> >>>           ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
> >>> @@ -289,6 +309,11 @@ for expr in exprs:
> >>>           fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
> >>>           ret += generate_type_cleanup_decl(expr['union'])
> >>>           fdef.write(generate_type_cleanup(expr['union']) + "\n")
> >>> +    elif expr.has_key('enum'):
> >>> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
> >>> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
> >>> +        ret += generate_type_cleanup_decl(expr['enum'])
> >>> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
> >>>       else:
> >>>           continue
> >>>       fdecl.write(ret)
> >>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> >>> index 8d4e94a..e44edfa 100644
> >>> --- a/scripts/qapi-visit.py
> >>> +++ b/scripts/qapi-visit.py
> >>> @@ -81,7 +81,7 @@ end:
> >>>   ''')
> >>>       return ret
> >>>
> >>> -def generate_visit_list(name, members):
> >>> +def generate_visit_list(name, members, enum=False):
> >>>       return mcgen('''
> >>>
> >>>   void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
> >>> @@ -160,12 +160,14 @@ end:
> >>>
> >>>       return ret
> >>>
> >>> -def generate_declaration(name, members, genlist=True):
> >>> -    ret = mcgen('''
> >>> +def generate_declaration(name, members, genlist=True, enum=False):
> >>> +    ret = ""
> >>> +    if not enum:
> >>> +        ret = mcgen('''
> 
> >>>   void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
> >>>   ''',
> >>> -                name=name)
> >>> +                    name=name)
> >>
> >> Why this change?
> 
> 
> Indent of above "mcgen(" changes, make them aligned.
> 
> >>>
> >>>       if genlist:
> >>>           ret += mcgen('''
> >>> @@ -293,10 +295,12 @@ for expr in exprs:
> >>>           ret += generate_declaration(expr['union'], expr['data'])
> >>>           fdecl.write(ret)
> >>>       elif expr.has_key('enum'):
> >>> -        ret = generate_visit_enum(expr['enum'], expr['data'])
> >>> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
> >>> +        ret += generate_visit_enum(expr['enum'], expr['data'])
> >>>           fdef.write(ret)
> >>>
> >>>           ret = generate_decl_enum(expr['enum'], expr['data'])
> >>> +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
> >>>           fdecl.write(ret)
> >>>
> >>>   fdecl.write('''
> >>
> >>
>