Eric Blake <ebl...@redhat.com> 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.
