On 11/09/2015 07:49 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Consolidate two common sequences of clash detection into a
>> new QAPISchemaObjectType.check_clash() helper method.
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>

>> @@ -980,11 +980,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>>          seen = OrderedDict()
>>          if self._base_name:
>>              self.base = schema.lookup_type(self._base_name)
>> -            assert isinstance(self.base, QAPISchemaObjectType)
>> -            assert not self.base.variants       # not implemented
>> -            self.base.check(schema)
>> -            for m in self.base.members:
>> -                m.check_clash(seen)
>> +            self.base.check_clash(schema, seen)
>>          for m in self.local_members:
>>              m.check(schema)
>>              m.check_clash(seen)
>> @@ -994,6 +990,12 @@ class QAPISchemaObjectType(QAPISchemaType):
>>              assert self.variants.tag_member in self.members
>>              self.variants.check_clash(schema, seen)
>>
>> +    def check_clash(self, schema, seen):
>> +        self.check(schema)
> 
> Do we want to hide this .check() inside .check_clash()?
> 
> QAPISchemaObjectTypeMember.check() doesn't.  I think the two better
> behave the same.
> 
>> +        assert not self.variants       # not implemented
>> +        for m in self.members:
>> +            m.check_clash(seen)

The self.check(schema) call is necessary to get self.members populated.
 We cannot iterate over self.members if the type has not had check()
called; this is true for both callers of type.check_clash()
(ObjectType.check(), and Variants.check_clash()).

You are correct that neither Member.check() nor Member.check_clash()
call a form of type.check() - but that's because at that level, there is
no need to populate a type.members list.

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org