Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 11/10/2015 02:15 AM, Markus Armbruster wrote:
>
>>> On the other hand, we've been arguing that check() should populate
>>> everything after construction prior to anything else being run; and not
>>> running Variant.type.check() during Variants.check() of flat unions
>>> feels like we may have a hole (a flat union will have to inline its
>>> types to the overall JSON object, and inlining types requires access to
>>> type.members - but as written, we aren't populating them until
>>> Variants.check_clash()).  I can play with hoisting the type.check() out
>>> of type.check_clash() and instead keep base.check() in type.check(), and
>>> add variant.type.check() in Variants.check() (but only for unions, not
>>> for alternates), if you are interested.
>> 
>> My "qapi: Factor out QAPISchemaObjectTypeMember.check_clash()" adds
>> QAPISchemaObjectTypeMember.check_clash() without changing the common
>> protocol.  The new QAPISchemaObjectTypeMember.check_clash() is merely a
>> helper for QAPISchemaObjectType.check().
>> 
>> The two .check_clash() you add (one in this patch, one in the previous
>> one) are different: both contain calls of QAPISchemaObjectType.check().
>> 
>> I feel the .check() calls are too important to be buried deep like that.
>> I'd stick to prior practice and put the .check() calls right into
>> .check().  Obviously, the .check_clash() methods may only called after
>> .check() then, but that's nothing new.
>> 
>> Fixup for your previous patch:
>> 
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 4c56935..357127d 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -1065,7 +1065,6 @@ class QAPISchemaObjectTypeVariants(object):
>>              vseen = dict(seen)
>>              assert isinstance(v.type, QAPISchemaObjectType)
>>              assert not v.type.variants       # not implemented
>> -            v.type.check(schema)
>>              for m in v.type.members:
>>                  m.check_clash(vseen)
>>  
>> @@ -1077,6 +1076,7 @@ class 
>> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>>      def check(self, schema, tag_type):
>>          QAPISchemaObjectTypeMember.check(self, schema)
>>          assert self.name in tag_type.values
>> +        self.type.check(schema)
>>  
>
> Won't quite work.  You are right that we must call
> self.type.check(schema) for variants used by a union; but calling it for
> ALL variants used by an alternate is wrong, because self.type for at
> least one branch of an alternate will not be an instance of
> QAPISchemaObjectType.  However, I'm currently testing whether it is safe
> to check to just blindly check an object branch of an alternate, if
> present (and that should not lead to cycles, since alternates have no
> base class and since we don't allow one alternate type as a variant of
> another alternate), in which case the fixup for 23/30 is more like:
>
> diff --git i/scripts/qapi.py w/scripts/qapi.py
> index a005c87..25fa642 100644
> --- i/scripts/qapi.py
> +++ w/scripts/qapi.py
> @@ -1065,7 +1065,6 @@ class QAPISchemaObjectTypeVariants(object):
>              vseen = dict(seen)
>              assert isinstance(v.type, QAPISchemaObjectType)
>              assert not v.type.variants       # not implemented
> -            v.type.check(schema)
>              for m in v.type.members:
>                  m.check_clash(vseen)
>
> @@ -1077,6 +1076,8 @@ class
> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      def check(self, schema, tag_type):
>          QAPISchemaObjectTypeMember.check(self, schema)
>          assert self.name in tag_type.values
> +        if isinstance(self.type, QAPISchemaObjectType):
> +            self.type.check(schema)
>
>      # This function exists to support ugly simple union special cases
>      # TODO get rid of them, and drop the function
> @@ -1098,6 +1099,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
>
>      def check(self, schema):
>          self.variants.tag_member.check(schema)
> +        # Not calling self.variants.check_clash(), because there's
> +        # nothing to clash with
>          self.variants.check(schema, {})
>
>      def json_type(self):

Makes sense to me.