On 11/09/2015 07:49 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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 <ebl...@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
signature.asc
Description: OpenPGP digital signature