Re: [Qemu-devel] [PATCH v10 25/30] qapi: Hoist tag collision check to Variants.check()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Checking that a given QAPISchemaObjectTypeVariant.name is a
> member of the corresponding QAPISchemaEnumType of the owning
> QAPISchemaObjectTypeVariants.tag_member ensures that there are
> no collisions in the generated C union for those tag values
> (since the enum itself should have no collisions).
>
> However, this check was the only thing that Variant.check() was
> doing beyond the work of the superclass ObjectTypeMember.check(),
> and resulted in a difference of the .check() signatures just to
> pass the enum type down.
>
> Simplify things by instead doing the tag name check as part of
> Variants.check(), at which point we can rely on inheritance
> instead of overriding Variant.check().
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v10: new patch
> ---
>  scripts/qapi.py | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 6d8c4c7..798df51 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1056,9 +1056,11 @@ class QAPISchemaObjectTypeVariants(object):
>      def check(self, schema, seen):
>          if self.tag_name:    # flat union
>              self.tag_member = seen[self.tag_name]
> -        assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> +        tag_type = self.tag_member.type
> +        assert isinstance(tag_type, QAPISchemaEnumType)
>          for v in self.variants:
> -            v.check(schema, self.tag_member.type)
> +            v.check(schema)
> +            assert v.name in tag_type.values

Two changes squashed together:

* Move the assertion from QAPISchemaObjectTypeVariant.check(),

* Capture self.tag_member.type in tag_type

The second part makes the patch slightly less obvious.  Matter of taste.

>
>      def check_clash(self, schema, seen):
>          for v in self.variants:
> @@ -1071,10 +1073,6 @@ class 
> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      def __init__(self, name, typ):
>          QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
>
> -    def check(self, schema, tag_type):
> -        QAPISchemaObjectTypeMember.check(self, schema)
> -        assert self.name in tag_type.values
> -
>      # This function exists to support ugly simple union special cases
>      # TODO get rid of them, and drop the function
>      def simple_union_type(self):