From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40978) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zwu4o-0004T2-MJ for qemu-devel@nongnu.org; Thu, 12 Nov 2015 10:46:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zwu4l-0001or-7D for qemu-devel@nongnu.org; Thu, 12 Nov 2015 10:46:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52297) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zwu4k-0001ok-Ti for qemu-devel@nongnu.org; Thu, 12 Nov 2015 10:46:15 -0500 From: Markus Armbruster References: <1447224690-9743-1-git-send-email-eblake@redhat.com> <1447224690-9743-28-git-send-email-eblake@redhat.com> Date: Thu, 12 Nov 2015 16:46:11 +0100 In-Reply-To: <1447224690-9743-28-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 10 Nov 2015 23:51:29 -0700") Message-ID: <87oaezi5ks.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [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: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > 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 Didn't we just drop this kind of implicit enum? > + else: > + return "Union '%s' branch" % owner.name > + else: > + return "Enum '%s' value" % self.name I like to call it "member" rather than value, because it avoids confusion with the numeric value of the C enumeration constant generated for it. The conditional isn't exactly elegant, but it would do. I'm not 100% convinced we need it, though. self.info already points to whatever defined this enum, either an explicit enum definition, or a simple union definition. How do the error messages come out if we dumb down to "Member '%s'"? A method with a similar purpose exists in QAPISchemaObjectTypeMember, but it's spelled describe(). It's used only from within the class. Rename it to match this one? > + > 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 Loop body is very similar to QAPISchemaObjectTypeMember.check_clash(). Differences: * c_enum_const(enum_name, member_name) vs. c_name(member_name).upper() This isn't really a difference, because the former returns the latter prefixed by a string that doesn't vary in the loop. One could argue that using c_enum_const() lets us abstract from what it does, but that's an illusion. We rely on it when we generate union members called c_name(member_name) without checking for collisions again. By the way, c_enum_const(self.name, value, self.prefix) would be more correct. Doesn't matter here, of course. Therefore, I'd be very much tempted to use c_name(member_name).upper() here as well. * The error message. But I suspect the same "Member '%s' clashes with '%s'" could do for both. If I'm right and we can drop the differences, the common code could become a helper function. > > 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' Would become tests/qapi-schema/enum-clash-member.json:2: Member 'ONE' clashes with 'one' Good enough for me. > 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)' tests/qapi-schema/enum-max-member.json:3: Member 'max' clashes with '(automatic MAX)' Good enough for me. > 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' tests/qapi-schema/union-clash-branches.json:4: Member 'a_b' clashes with 'a-b' Good enough for me. > 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)' tests/qapi-schema/union-max.json:2: Member 'max' clashes with '(automatic MAX)' Good enough for me.