diff mbox

[v11,27/28] qapi: Move duplicate enum value checks to schema check()

Message ID 1447224690-9743-28-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Nov. 11, 2015, 6:51 a.m. UTC
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 <eblake@redhat.com>

---
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

Comments

Markus Armbruster Nov. 12, 2015, 3:46 p.m. UTC | #1
Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>
> ---
> 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.
Eric Blake Nov. 12, 2015, 4:08 p.m. UTC | #2
On 11/12/2015 08:46 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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).
>>

>> @@ -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?

D'oh; rebase churn reordering patches.

> 
>> +            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.

Sure, that sounds slightly better.

> 
> 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.

Are you talking about _pretty_owner()? Or the actual describe() that
calls _pretty_owner()?

> 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.

Well, it _is_ a difference if c_name() munges differently than
c_enum_const() (the whole question of whether 'OneTwo' and 'One-Two' are
detected as clashes); but then again, I have the patches that try to
rework c_enum_const() to use just c_name().upper().

> 
>   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.

I'll play with it and see what pops out.
Eric Blake Nov. 18, 2015, 6:48 a.m. UTC | #3
On 11/12/2015 08:46 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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).
>>

>> +    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?

[as much notes to myself as anything else...]

We chatted some on IRC, and had some more ideas, thus I will delay
patches 26-28 to a later date while posting the rest of v12.  Among
those ideas
 - getting rid of the enum _MAX clash will simplify things (to be done
in v12)
 - create a new QAPISchemaMember class as the superclass of
QAPISchemaObjectTypeMember, and have enum members be instances of this
type rather than a straight string (at which point the describe() method
and collision checking lives in the superclass, while the associated
type only lives in the subclass). Less code duplication that way
 - the idea of multiline error messages, only when info !=
member.owner.info; as in:
foo.json:4: Member 'one' clashes with 'One'
foo.json:2: Member 'one' defined here
diff mbox

Patch

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)'