Eric Blake <ebl...@redhat.com> writes: > 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()).
Yes. We have a common protocol for QAPISchemaFOO objects, namely that certain instance variables and methods are only valid after .check(). > 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. My "qapi: Factor out QAPISchemaObjectTypeMember.check_clash()" added QAPISchemaObjectTypeMember.check_clash() without changing the common protocol. The new QAPISchemaObjectTypeMember.check_clash() is merely a helper for QAPISchemaObjectType.check(). Your Gcc: nnml:mail.redhat.xlst.qemu-devel From: Markus Armbruster <arm...@redhat.com> --text follows this line-- Eric Blake <ebl...@redhat.com> writes: > 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()). Yes. We have a common protocol for QAPISchemaFOO objects, namely that certain instance variables and methods are only valid after .check(). > 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. 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) # This function exists to support ugly simple union special cases # TODO get rid of them, and drop the function This patch with conflicts resolved and the same idea worked in then becomes: diff --git a/scripts/qapi.py b/scripts/qapi.py index 357127d..f443ec5 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -981,10 +981,8 @@ class QAPISchemaObjectType(QAPISchemaType): 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 +992,11 @@ class QAPISchemaObjectType(QAPISchemaType): assert self.variants.tag_member in self.members self.variants.check_clash(schema, seen) + def check_clash(self, schema, seen): + assert not self.variants # not implemented + for m in self.members: + m.check_clash(seen) + def is_implicit(self): # See QAPISchema._make_implicit_object_type() return self.name[0] == ':' @@ -1062,11 +1065,8 @@ class QAPISchemaObjectTypeVariants(object): for v in self.variants: # Reset seen map for each variant, since qapi names from one # branch do not affect another branch - vseen = dict(seen) assert isinstance(v.type, QAPISchemaObjectType) - assert not v.type.variants # not implemented - for m in v.type.members: - m.check_clash(vseen) + v.type.check_clash(schema, dict(seen)) class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): Your two check_clash() are then simple helpers, just like mine.