From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45209) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGdok-0001XS-Ak for qemu-devel@nongnu.org; Thu, 20 Feb 2014 19:18:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WGdob-0003qD-1s for qemu-devel@nongnu.org; Thu, 20 Feb 2014 19:18:14 -0500 Received: from e23smtp07.au.ibm.com ([202.81.31.140]:39701) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGdoa-0003pu-6l for qemu-devel@nongnu.org; Thu, 20 Feb 2014 19:18:04 -0500 Received: from /spool/local by e23smtp07.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 21 Feb 2014 10:18:01 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id BB48E3578052 for ; Fri, 21 Feb 2014 11:17:59 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s1KNwCNI9699634 for ; Fri, 21 Feb 2014 10:58:12 +1100 Received: from d23av03.au.ibm.com (localhost [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s1L0Hx5R022724 for ; Fri, 21 Feb 2014 11:17:59 +1100 Message-ID: <53069B34.2060006@linux.vnet.ibm.com> Date: Fri, 21 Feb 2014 08:17:56 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1392875695-15627-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1392875695-15627-8-git-send-email-xiawenc@linux.vnet.ibm.com> <87a9dlwxh9.fsf@blackfin.pond.sub.org> In-Reply-To: <87a9dlwxh9.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=GB2312 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com ÓÚ 2014/2/21 0:38, Markus Armbruster дµÀ: > Wenchao Xia 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 >> --- >> 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.