δΊ 2014/2/21 0:38, Markus Armbruster ει: > Wenchao Xia <xiaw...@linux.vnet.ibm.com> writes: > >> By default, any union will automatically generate a enum type as >> "[UnionName]Kind" in C code, and it is duplicated when the discriminator >> is specified as a pre-defined enum type in schema. After this patch, >> the pre-defined enum type will be really used as the switch case >> condition in generated C code, if discriminator is an enum field. >> >> Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com> >> --- >> docs/qapi-code-gen.txt | 8 ++++++-- >> scripts/qapi-types.py | 20 ++++++++++++++++---- >> scripts/qapi-visit.py | 27 ++++++++++++++++++++------- >> scripts/qapi.py | 13 ++++++++++++- >> 4 files changed, 54 insertions(+), 14 deletions(-) >> >> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt >> index 0728f36..a2e7921 100644 >> --- a/docs/qapi-code-gen.txt >> +++ b/docs/qapi-code-gen.txt >> @@ -123,11 +123,15 @@ And it looks like this on the wire: >> >> Flat union types avoid the nesting on the wire. They are used whenever a >> specific field of the base type is declared as the discriminator ('type' is >> -then no longer generated). The discriminator must always be a string field. >> +then no longer generated). The discriminator can be a string field or a >> +predefined enum field. If it is a string field, a hidden enum type will be >> +generated as "[UNION_NAME]Kind". If it is an enum field, a compile time >> check >> +will be done to verify the correctness. It is recommended to use an enum >> field. >> The above example can then be modified as follows: >> >> + { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] } >> { 'type': 'BlockdevCommonOptions', >> - 'data': { 'driver': 'str', 'readonly': 'bool' } } >> + 'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } } >> { 'union': 'BlockdevOptions', >> 'base': 'BlockdevCommonOptions', >> 'discriminator': 'driver', >> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py >> index 656a9a0..4098c60 100644 >> --- a/scripts/qapi-types.py >> +++ b/scripts/qapi-types.py >> @@ -201,14 +201,22 @@ def generate_union(expr): >> base = expr.get('base') >> discriminator = expr.get('discriminator') >> >> + expr_elem = {'expr': expr} >> + enum_define = discriminator_find_enum_define(expr_elem) > > expr_elem has no fp, line. What if discriminator_find_enum_define > throws a QAPIExprError? > It shouldn't happen, since all error check happens in parse_schema().
> More of the same below. > >> + if enum_define: >> + discriminator_type_name = enum_define['enum_name'] >> + else: >> + discriminator_type_name = '%sKind' % (name) >> + >> ret = mcgen(''' >> struct %(name)s >> { >> - %(name)sKind kind; >> + %(discriminator_type_name)s kind; >> union { >> void *data; >> ''', >> - name=name) >> + name=name, >> + discriminator_type_name=discriminator_type_name) >> >> for key in typeinfo: >> ret += mcgen(''' >> @@ -389,8 +397,12 @@ for expr in exprs: >> fdef.write(generate_enum_lookup(expr['enum'], expr['data'])) >> elif expr.has_key('union'): >> ret += generate_fwd_struct(expr['union'], expr['data']) + "\n" >> - ret += generate_enum('%sKind' % expr['union'], expr['data'].keys()) >> - fdef.write(generate_enum_lookup('%sKind' % expr['union'], >> expr['data'].keys())) >> + expr_elem = {'expr': expr} >> + enum_define = discriminator_find_enum_define(expr_elem) >> + if not enum_define: >> + ret += generate_enum('%sKind' % expr['union'], >> expr['data'].keys()) >> + fdef.write(generate_enum_lookup('%sKind' % expr['union'], >> + expr['data'].keys())) > > Generate the implicit enum only when we don't have an explicit enum > discriminator. Good. > >> if expr.get('discriminator') == {}: >> fdef.write(generate_anon_union_qtypes(expr)) >> else: >> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py >> index 87e6df7..08685a7 100644 >> --- a/scripts/qapi-visit.py >> +++ b/scripts/qapi-visit.py >> @@ -260,10 +260,17 @@ def generate_visit_union(expr): >> assert not base >> return generate_visit_anon_union(name, members) >> >> - # 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()) >> - discriminator_type_name = '%sKind' % (name) >> + expr_elem = {'expr': expr} >> + enum_define = discriminator_find_enum_define(expr_elem) >> + if enum_define: >> + # Use the predefined enum type as discriminator >> + ret = "" >> + discriminator_type_name = 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()) >> + discriminator_type_name = '%sKind' % (name) > > Generate the visit of the discriminator only when we don't have an > explicit enum discriminator (which gets visited elsewhere already). > Good. > >> >> if base: >> base_fields = find_struct(base)['data'] >> @@ -303,11 +310,12 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, >> const char *name, Error ** >> else: >> desc_type = discriminator >> ret += mcgen(''' >> - visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err); >> + visit_type_%(discriminator_type_name)s(m, &(*obj)->kind, >> "%(type)s", &err); > > Another long line due to very long identifier. > Will improve. >> if (!err) { >> switch ((*obj)->kind) { >> ''', >> - name=name, type=desc_type) >> + discriminator_type_name=discriminator_type_name, >> + type=desc_type) >> >> for key in members: >> if not discriminator: >> @@ -519,7 +527,12 @@ for expr in exprs: >> ret += generate_visit_list(expr['union'], expr['data']) >> fdef.write(ret) >> >> - ret = generate_decl_enum('%sKind' % expr['union'], >> expr['data'].keys()) >> + expr_elem = {'expr': expr} >> + enum_define = discriminator_find_enum_define(expr_elem) >> + ret = "" >> + if not enum_define: >> + ret = generate_decl_enum('%sKind' % expr['union'], >> + expr['data'].keys()) > > Generate the visitor for the implicit enum only when we don't have an > explicit enum discriminator (which has its own visitor already). Good. > >> ret += generate_declaration(expr['union'], expr['data']) >> fdecl.write(ret) >> elif expr.has_key('enum'): >> diff --git a/scripts/qapi.py b/scripts/qapi.py >> index 130dced..2a5eb59 100644 >> --- a/scripts/qapi.py >> +++ b/scripts/qapi.py >> @@ -250,11 +250,22 @@ def parse_schema(fp): >> add_enum(expr['enum'], expr['data']) >> elif expr.has_key('union'): >> add_union(expr) >> - add_enum('%sKind' % expr['union']) >> elif expr.has_key('type'): >> add_struct(expr) >> exprs.append(expr) >> >> + # Try again for hidden UnionKind enum >> + for expr_elem in schema.exprs: >> + expr = expr_elem['expr'] >> + if expr.has_key('union'): >> + try: >> + enum_define = discriminator_find_enum_define(expr_elem) >> + except QAPIExprError, e: >> + print >>sys.stderr, e >> + exit(1) >> + if not enum_define: >> + add_enum('%sKind' % expr['union']) >> + >> try: >> check_exprs(schema) >> except QAPIExprError, e: > > I guess you move this into its own loop because when base types are used > before they're defined, or an enum type is used for a discriminator > before it's defined, then discriminator_find_enum_define() complains. > Correct? > Exactly, which allow enum define after usage in schema.