From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41546) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwPGH-0006hY-DB for qemu-devel@nongnu.org; Wed, 11 Nov 2015 01:52:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwPG0-0005g2-OX for qemu-devel@nongnu.org; Wed, 11 Nov 2015 01:52:05 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47811) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwPG0-0005fc-EJ for qemu-devel@nongnu.org; Wed, 11 Nov 2015 01:51:48 -0500 From: Eric Blake Date: Tue, 10 Nov 2015 23:51:29 -0700 Message-Id: <1447224690-9743-28-git-send-email-eblake@redhat.com> In-Reply-To: <1447224690-9743-1-git-send-email-eblake@redhat.com> References: <1447224690-9743-1-git-send-email-eblake@redhat.com> Subject: [Qemu-devel] [PATCH v11 27/28] 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: armbru@redhat.com, Michael Roth 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 mapping to the same enum value, even when the names do not collide case-wise. 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 wasn't adding much: union-clash-branches exposes the error message when branches directly collide, and union-max exposes the error message when branches cause an enum collision (union-bad-branch basically causes an enum collision that would not be a C collision). This goes along with our desire to require ALL names to be case-insensitively unique. No change to generated code. Signed-off-by: Eric Blake --- v11 (no v10): message of union-clash-branches.err changed: rely on enum check rather than Variant.check() to catch it v9: rebase to earlier changes, update commit message, break out helper _describe() method v8: rebase to earlier changes; better comments v7: retitle and improve commit message; earlier subclass patches avoid problem with detecting 'kind' collision v6: new patch --- scripts/qapi.py | 41 ++++++++++++++++------------- tests/Makefile | 1 - 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-branches.json | 2 +- tests/qapi-schema/union-max.err | 2 +- 11 files changed, 28 insertions(+), 34 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 1d59ce9..08a366e 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -533,7 +533,6 @@ def check_union(expr, expr_info): base = expr.get('base') discriminator = expr.get('discriminator') members = expr['data'] - values = {'MAX': '(automatic)'} # Two types of unions, determined by discriminator. @@ -593,15 +592,6 @@ 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'] @@ -630,7 +620,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, @@ -641,12 +630,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): @@ -873,8 +856,30 @@ class QAPISchemaEnumType(QAPISchemaType): self.values = values self.prefix = prefix + def _describe(self, schema): + # 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): + return "Alternate '%s' branch" % owner.name + else: + return "Union '%s' branch" % owner.name + else: + return "Enum '%s' value" % self.name + 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: + raise QAPIExprError(self.info, + "%s '%s' clashes with '%s'" + % (self._describe(schema), 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 d1c6817..cdff7a4 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -336,7 +336,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/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-branches.json b/tests/qapi-schema/union-clash-branches.json index 31d135f..3bece8c 100644 --- a/tests/qapi-schema/union-clash-branches.json +++ b/tests/qapi-schema/union-clash-branches.json @@ -1,5 +1,5 @@ # Union branch name collision # Reject a union that would result in a collision in generated C names (this -# would try to generate two enum values 'TEST_UNION_KIND_A_B'). +# would try to generate two members 'a_b'). { 'union': 'TestUnion', 'data': { 'a-b': 'int', 'a_b': 'str' } } 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