Eric Blake <ebl...@redhat.com> writes:
> Now that c_var() handles '.' in downstream extension names, fix
> the generator to support such names as additional types, enums,
> members within an enum, branches of a union or alternate, and
> in arrays.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> scripts/qapi-commands.py | 4 ++--
> scripts/qapi-types.py | 27 ++++++++++++++-------------
> scripts/qapi-visit.py | 44 ++++++++++++++++++++++++--------------------
> scripts/qapi.py | 10 ++++++----
> 4 files changed, 46 insertions(+), 39 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index af1e1a1..84b66bc 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -22,9 +22,9 @@ import errno
>
> def type_visitor(name):
> if type(name) == list:
> - return 'visit_type_%sList' % name[0]
> + return 'visit_type_%sList' % type_name(name[0])
> else:
> - return 'visit_type_%s' % name
> + return 'visit_type_%s' % type_name(name)
>
> def generate_command_decl(name, args, ret_type):
> arglist=""
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 8cf6349..b33b8fd 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -45,7 +45,7 @@ typedef struct %(name)sList
> struct %(name)sList *next;
> } %(name)sList;
> ''',
> - name=name)
> + name=c_var(name))
>
> def generate_fwd_enum_struct(name, members):
> return mcgen('''
> @@ -58,7 +58,7 @@ typedef struct %(name)sList
> struct %(name)sList *next;
> } %(name)sList;
> ''',
> - name=name)
> + name=c_var(name))
>
> def generate_struct_fields(members):
> ret = ''
> @@ -87,7 +87,7 @@ def generate_struct(expr):
> struct %(name)s
> {
> ''',
> - name=structname)
> + name=c_var(structname))
>
> if base:
> ret += generate_struct_fields({'base': base})
> @@ -115,7 +115,7 @@ def generate_enum_lookup(name, values):
> ret = mcgen('''
> const char *%(name)s_lookup[] = {
> ''',
> - name=name)
> + name=c_var(name))
> i = 0
> for value in values:
> index = generate_enum_full_value(name, value)
> @@ -134,6 +134,7 @@ const char *%(name)s_lookup[] = {
> return ret
>
> def generate_enum(name, values):
> + name = c_var(name)
> lookup_decl = mcgen('''
> extern const char *%(name)s_lookup[];
> ''',
> @@ -173,18 +174,18 @@ def generate_alternate_qtypes(expr):
> ret = mcgen('''
> const int %(name)s_qtypes[QTYPE_MAX] = {
> ''',
> - name=name)
> + name=c_var(name))
>
> for key in members:
> qtype = find_alternate_member_qtype(members[key])
> assert qtype, "Invalid alternate member"
>
> ret += mcgen('''
> - [ %(qtype)s ] = %(abbrev)s_KIND_%(enum)s,
> + [%(qtype)s] = %(value)s,
> ''',
> - qtype = qtype,
> - abbrev = de_camel_case(name).upper(),
> - enum = c_var(de_camel_case(key),False).upper())
> + qtype = qtype,
> + value = generate_enum_full_value("%sKind" %c_var(name),
> + key))
>
> ret += mcgen('''
> };
I fixed this one in my "[PATCH RFC 06/19] qapi: Use c_enum_const() in
generate_alternate_qtypes()".
You picked just my "[PATCH RFC 02/19] qapi: Fix C identifiers generated
for names containing '.'" into your series. Would you mind picking all
the related patches, i.e PATCH 02-07?
qapi: Fix C identifiers generated for names containing '.'
qapi: Rename _generate_enum_string() to camel_to_upper()
qapi: Rename generate_enum_full_value() to c_enum_const()
qapi: Simplify c_enum_const()
qapi: Use c_enum_const() in generate_alternate_qtypes()
qapi: Move camel_to_upper(), c_enum_const() to closely related code
> @@ -194,7 +195,7 @@ const int %(name)s_qtypes[QTYPE_MAX] = {
>
> def generate_union(expr, meta):
>
> - name = expr[meta]
> + name = c_var(expr[meta])
> typeinfo = expr['data']
>
> base = expr.get('base')
> @@ -214,7 +215,7 @@ struct %(name)s
> void *data;
> ''',
> name=name,
> - discriminator_type_name=discriminator_type_name)
> + discriminator_type_name=c_var(discriminator_type_name))
>
> for key in typeinfo:
> ret += mcgen('''
> @@ -251,7 +252,7 @@ def generate_type_cleanup_decl(name):
> ret = mcgen('''
> void qapi_free_%(type)s(%(c_type)s obj);
> ''',
> - c_type=c_type(name),type=name)
> + c_type=c_type(name),type=c_var(name))
> return ret
>
> def generate_type_cleanup(name):
> @@ -272,7 +273,7 @@ void qapi_free_%(type)s(%(c_type)s obj)
> qapi_dealloc_visitor_cleanup(md);
> }
> ''',
> - c_type=c_type(name),type=name)
> + c_type=c_type(name),type=c_var(name))
> return ret
>
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index f24dcfa..d1d3fe6 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -44,12 +44,13 @@ static void visit_type_implicit_%(c_type)s(Visitor *m,
> %(c_type)s **obj, Error *
> c_type=type_name(type))
>
> def generate_visit_struct_fields(name, field_prefix, fn_prefix, members,
> base = None):
> + assert field_prefix == ""
Makes me wonder why we have a field_prefix parameter.
fn_prefix is also always ""...
> substructs = []
> ret = ''
> if not fn_prefix:
> - full_name = name
> + full_name = c_var(name)
> else:
... and therefore this this code is unreachable.
Just observing, not asking you to do anything here.
> - full_name = "%s_%s" % (name, fn_prefix)
> + full_name = "%s_%s" % (c_var(name), fn_prefix)
>
> if base:
> ret += generate_visit_implicit_struct(base)
> @@ -60,7 +61,7 @@ static void visit_type_%(full_name)s_fields(Visitor *m,
> %(name)s **obj, Error **
> {
> Error *err = NULL;
> ''',
> - name=name, full_name=full_name)
> + name=c_var(name), full_name=full_name)
> push_indent()
>
> if base:
> @@ -121,9 +122,9 @@ def generate_visit_struct_body(field_prefix, name,
> members):
> ''')
>
> if not field_prefix:
> - full_name = name
> + full_name = c_var(name)
> else:
> - full_name = "%s_%s" % (field_prefix, name)
> + full_name = "%s_%s" % (field_prefix, c_var(name))
>
> if len(field_prefix):
> ret += mcgen('''
> @@ -132,9 +133,9 @@ def generate_visit_struct_body(field_prefix, name,
> members):
> name=name)
> else:
> ret += mcgen('''
> - visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s),
> &err);
> + visit_start_struct(m, (void **)obj, "%(name)s", name,
> sizeof(%(c_name)s), &err);
> ''',
> - name=name)
> + name=name, c_name=c_var(name))
>
> ret += mcgen('''
> if (!err) {
> @@ -162,7 +163,7 @@ def generate_visit_struct(expr):
> void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error
> **errp)
> {
> ''',
> - name=name)
> + name=c_var(name))
>
> ret += generate_visit_struct_body("", name, members)
>
> @@ -171,7 +172,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj,
> const char *name, Error **e
> ''')
> return ret
>
> -def generate_visit_list(name, members):
> +def generate_visit_list(name, members, builtin=False):
> + if not builtin:
> + name = c_var(name)
Fun.
c_var() does two things:
(a) it protects certain words if protect=True
(b) it maps funny characters to '_'.
When builtin, (a) is unwanted, and (b) does nothing. That's why we need
the conditional.
A possible alternative could be c_var(name, not builtin). Matter of
taste.
Hmm, just saw what type_name() does. Why not just
name = type_name(name)
?
> return mcgen('''
>
> void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char
> *name, Error **errp)
> @@ -208,7 +211,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s *obj, const
> char *name, Error **er
> visit_type_enum(m, (int *)obj, %(name)s_lookup, "%(name)s", name, errp);
> }
> ''',
> - name=name)
> + name=c_var(name))
>
> def generate_visit_alternate(name, members):
> ret = mcgen('''
> @@ -227,11 +230,11 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj,
> const char *name, Error **e
> }
> switch ((*obj)->kind) {
> ''',
> - name=name)
> + name=c_var(name))
>
> # For alternate, always use the default enum type automatically generated
> # as "'%sKind' % (name)"
> - disc_type = '%sKind' % (name)
> + disc_type = '%sKind' % c_var(name)
>
> for key in members:
> assert (members[key] in builtin_types.keys()
> @@ -277,12 +280,12 @@ def generate_visit_union(expr):
> if enum_define:
> # Use the enum type as discriminator
> ret = ""
> - disc_type = enum_define['enum_name']
> + disc_type = c_var(enum_define['enum_name'])
> else:
> # There will always be a discriminator in the C switch code, by
> default
> # it is an enum type generated silently as "'%sKind' % (name)"
> ret = generate_visit_enum('%sKind' % name, members.keys())
> - disc_type = '%sKind' % (name)
> + disc_type = '%sKind' % c_var(name)
>
> if base:
> assert discriminator
> @@ -306,7 +309,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj,
> const char *name, Error **e
> }
> if (*obj) {
> ''',
> - name=name)
> + name=c_var(name))
>
> if base:
> ret += mcgen('''
> @@ -315,7 +318,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj,
> const char *name, Error **e
> goto out_obj;
> }
> ''',
> - name=name)
> + name=c_var(name))
>
> if not discriminator:
> disc_key = "type"
> @@ -372,6 +375,7 @@ out:
> def generate_declaration(name, members, builtin_type=False):
> ret = ""
> if not builtin_type:
> + name = c_var(name)
> ret += mcgen('''
>
> void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error
> **errp);
> @@ -381,7 +385,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj,
> const char *name, Error **e
> ret += mcgen('''
> void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char
> *name, Error **errp);
> ''',
> - name=name)
> + name=name)
>
> return ret
>
> @@ -389,7 +393,7 @@ def generate_enum_declaration(name, members):
> ret = mcgen('''
> void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char
> *name, Error **errp);
> ''',
> - name=name)
> + name=c_var(name))
>
> return ret
>
> @@ -398,7 +402,7 @@ def generate_decl_enum(name, members):
>
> void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error
> **errp);
> ''',
> - name=name)
> + name=c_var(name))
>
> try:
> opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:i:o:",
> @@ -515,7 +519,7 @@ fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL"))
> # over these cases
> if do_builtins:
> for typename in builtin_types.keys():
> - fdef.write(generate_visit_list(typename, None))
> + fdef.write(generate_visit_list(typename, None, builtin=True))
>
> for expr in exprs:
> if expr.has_key('struct'):
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 1d64c62..e706712 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -782,12 +782,14 @@ def c_var(name, protect=True):
> return name.translate(c_var_trans)
>
> def c_list_type(name):
> - return '%sList' % name
> + return '%sList' % type_name(name)
>
> def type_name(name):
> if type(name) == list:
> return c_list_type(name[0])
> - return name
> + if name in builtin_types.keys():
> + return name
> + return c_var(name)
>
> def add_name(name, info, meta, implicit = False):
> global all_names
> @@ -869,13 +871,13 @@ def c_type(name, is_param=False):
> elif type(name) == list:
> return '%s *%s' % (c_list_type(name[0]), eatspace)
> elif is_enum(name):
> - return name
> + return c_var(name)
> elif name == None or len(name) == 0:
> return 'void'
> elif name in events:
> return '%sEvent *%s' % (camel_case(name), eatspace)
> else:
> - return '%s *%s' % (name, eatspace)
> + return '%s *%s' % (c_var(name), eatspace)
>
> def is_c_ptr(name):
> suffix = "*" + eatspace
If it was my patch, I'd be tempted to split it up some. Matter of
taste, feel free to keep it a single patch.
