From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39392) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZiaAz-0006zS-5T for qemu-devel@nongnu.org; Sat, 03 Oct 2015 23:41:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZiaAx-0002Bn-IR for qemu-devel@nongnu.org; Sat, 03 Oct 2015 23:41:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47726) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZiaAx-0002Bf-AK for qemu-devel@nongnu.org; Sat, 03 Oct 2015 23:41:27 -0400 From: Eric Blake Date: Sat, 3 Oct 2015 21:41:11 -0600 Message-Id: <1443930073-19359-13-git-send-email-eblake@redhat.com> In-Reply-To: <1443930073-19359-1-git-send-email-eblake@redhat.com> References: <1443930073-19359-1-git-send-email-eblake@redhat.com> Subject: [Qemu-devel] [PATCH v7 12/14] qapi: Move duplicate enum value checks to schema check() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Michael Roth , marcandre.lureau@redhat.com, armbru@redhat.com, ehabkost@redhat.com Similar to the previous commit, move the detection of a collision in enum values from parse time to QAPISchemaEnumType.check(). This happens to also detect collisions in union branch names, so for a decent error message, we have to determine if the enum is implicit (and if so where the real collision lies). Testing this showed that the test union-bad-branch and union-clash-branches were basically testing the same thing; with the minor difference that the former clashes only in the enum, while the latter also clashes in the C union member names that would be generated. So delete the weaker test. No change to generated code. Signed-off-by: Eric Blake --- v7: retitle and improve commit message; earlier subclass patches avoid problem with detecting 'kind' collision v6: new patch --- scripts/qapi.py | 47 +++++++++++++----------------- tests/Makefile | 1 - tests/qapi-schema/alternate-clash.err | 2 +- tests/qapi-schema/enum-clash-member.err | 2 +- tests/qapi-schema/enum-max-member.err | 2 +- tests/qapi-schema/union-bad-branch.err | 1 - tests/qapi-schema/union-bad-branch.exit | 1 - tests/qapi-schema/union-bad-branch.json | 8 ----- tests/qapi-schema/union-bad-branch.out | 0 tests/qapi-schema/union-clash-branches.err | 2 +- tests/qapi-schema/union-clash-type.err | 2 +- tests/qapi-schema/union-max.err | 2 +- 12 files changed, 26 insertions(+), 44 deletions(-) delete mode 100644 tests/qapi-schema/union-bad-branch.err delete mode 100644 tests/qapi-schema/union-bad-branch.exit delete mode 100644 tests/qapi-schema/union-bad-branch.json delete mode 100644 tests/qapi-schema/union-bad-branch.out diff --git a/scripts/qapi.py b/scripts/qapi.py index 9f98413..55aca40 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -530,7 +530,6 @@ def check_union(expr, expr_info): base = expr.get('base') discriminator = expr.get('discriminator') members = expr['data'] - values = {'MAX': '(automatic)', 'KIND': '(automatic)'} # Two types of unions, determined by discriminator. @@ -590,34 +589,16 @@ def check_union(expr, expr_info): "enum '%s'" % (key, enum_define["enum_name"])) - # Otherwise, check for conflicts in the generated enum - else: - c_key = camel_to_upper(key) - if c_key in values: - raise QAPIExprError(expr_info, - "Union '%s' member '%s' clashes with '%s'" - % (name, key, values[c_key])) - values[c_key] = key - def check_alternate(expr, expr_info): name = expr['alternate'] members = expr['data'] - values = {'MAX': '(automatic)'} types_seen = {} # Check every branch for (key, value) in members.items(): check_name(expr_info, "Member of alternate '%s'" % name, key) - # Check for conflicts in the generated enum - c_key = camel_to_upper(key) - if c_key in values: - raise QAPIExprError(expr_info, - "Alternate '%s' member '%s' clashes with '%s'" - % (name, key, values[c_key])) - values[c_key] = key - # Ensure alternates have no type conflicts. check_type(expr_info, "Member '%s' of alternate '%s'" % (key, name), value, @@ -636,7 +617,6 @@ def check_enum(expr, expr_info): name = expr['enum'] members = expr.get('data') prefix = expr.get('prefix') - values = {'MAX': '(automatic)'} if not isinstance(members, list): raise QAPIExprError(expr_info, @@ -647,12 +627,6 @@ def check_enum(expr, expr_info): for member in members: check_name(expr_info, "Member of enum '%s'" % name, member, enum_member=True) - key = camel_to_upper(member) - if key in values: - raise QAPIExprError(expr_info, - "Enum '%s' member '%s' clashes with '%s'" - % (name, member, values[key])) - values[key] = member def check_struct(expr, expr_info): @@ -887,7 +861,26 @@ class QAPISchemaEnumType(QAPISchemaType): self.prefix = prefix def check(self, schema): - assert len(set(self.values)) == len(self.values) + # Check for collisions on the generated C enum values + seen = {c_enum_const(self.name, 'MAX'): '(automatic MAX)'} + for value in self.values: + c_value = c_enum_const(self.name, value) + if c_value in seen: + # If the enum is implicit, report the error on behalf of + # the union or alternate that triggered the enum + if self.is_implicit(): + owner = schema.lookup_type(self.name[:-4]) + assert owner + if isinstance(owner, QAPISchemaAlternateType): + description = "Alternate '%s' branch" % owner.name + else: + description = "Union '%s' branch" % owner.name + else: + description = "Enum '%s' value" % self.name + raise QAPIExprError(self.info, + "%s '%s' clashes with '%s'" + % (description, value, seen[c_value])) + seen[c_value] = value def _is_implicit(self): # See QAPISchema._make_implicit_enum_type() diff --git a/tests/Makefile b/tests/Makefile index d18b3b2..b85ce48 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -331,7 +331,6 @@ qapi-schema += unclosed-list.json qapi-schema += unclosed-object.json qapi-schema += unclosed-string.json qapi-schema += unicode-str.json -qapi-schema += union-bad-branch.json qapi-schema += union-base-no-discriminator.json qapi-schema += union-clash-branches.json qapi-schema += union-clash-data.json diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err index a475ab6..7fd3069 100644 --- a/tests/qapi-schema/alternate-clash.err +++ b/tests/qapi-schema/alternate-clash.err @@ -1 +1 @@ -tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' clashes with 'a-b' +tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b' diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err index 48bd136..84030c5 100644 --- a/tests/qapi-schema/enum-clash-member.err +++ b/tests/qapi-schema/enum-clash-member.err @@ -1 +1 @@ -tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE' clashes with 'one' +tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' value 'ONE' clashes with 'one' diff --git a/tests/qapi-schema/enum-max-member.err b/tests/qapi-schema/enum-max-member.err index f77837f..6b9ef9b 100644 --- a/tests/qapi-schema/enum-max-member.err +++ b/tests/qapi-schema/enum-max-member.err @@ -1 +1 @@ -tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' member 'max' clashes with '(automatic)' +tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' value 'max' clashes with '(automatic MAX)' diff --git a/tests/qapi-schema/union-bad-branch.err b/tests/qapi-schema/union-bad-branch.err deleted file mode 100644 index 8822735..0000000 --- a/tests/qapi-schema/union-bad-branch.err +++ /dev/null @@ -1 +0,0 @@ -tests/qapi-schema/union-bad-branch.json:6: Union 'MyUnion' member 'ONE' clashes with 'one' diff --git a/tests/qapi-schema/union-bad-branch.exit b/tests/qapi-schema/union-bad-branch.exit deleted file mode 100644 index d00491f..0000000 --- a/tests/qapi-schema/union-bad-branch.exit +++ /dev/null @@ -1 +0,0 @@ -1 diff --git a/tests/qapi-schema/union-bad-branch.json b/tests/qapi-schema/union-bad-branch.json deleted file mode 100644 index 913aa38..0000000 --- a/tests/qapi-schema/union-bad-branch.json +++ /dev/null @@ -1,8 +0,0 @@ -# we reject normal unions where branches would collide in C -{ 'struct': 'One', - 'data': { 'string': 'str' } } -{ 'struct': 'Two', - 'data': { 'number': 'int' } } -{ 'union': 'MyUnion', - 'data': { 'one': 'One', - 'ONE': 'Two' } } diff --git a/tests/qapi-schema/union-bad-branch.out b/tests/qapi-schema/union-bad-branch.out deleted file mode 100644 index e69de29..0000000 diff --git a/tests/qapi-schema/union-clash-branches.err b/tests/qapi-schema/union-clash-branches.err index 005c48d..d8f1265 100644 --- a/tests/qapi-schema/union-clash-branches.err +++ b/tests/qapi-schema/union-clash-branches.err @@ -1 +1 @@ -tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b' +tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' branch 'a_b' clashes with 'a-b' diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err index a5dead1..ce7f8cd 100644 --- a/tests/qapi-schema/union-clash-type.err +++ b/tests/qapi-schema/union-clash-type.err @@ -1 +1 @@ -tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion' member 'kind' clashes with '(automatic)' +tests/qapi-schema/union-clash-type.json:8: 'kind' (branch of TestUnion) collides with 'kind' (implicit tag of TestUnion) diff --git a/tests/qapi-schema/union-max.err b/tests/qapi-schema/union-max.err index 55ce439..b93beae 100644 --- a/tests/qapi-schema/union-max.err +++ b/tests/qapi-schema/union-max.err @@ -1 +1 @@ -tests/qapi-schema/union-max.json:2: Union 'Union' member 'max' clashes with '(automatic)' +tests/qapi-schema/union-max.json:2: Union 'Union' branch 'max' clashes with '(automatic MAX)' -- 2.4.3