| Message ID | 1446791754-23823-25-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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> > > --- > v10: rebase on new Variants.check_clash() > v9: new patch, split off from v8 7/17 > --- > scripts/qapi.py | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 4c56935..6d8c4c7 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -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) This assertion is lost. > - 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) > + 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,12 +1064,7 @@ 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) This assertion is lost. > - assert not v.type.variants # not implemented > - v.type.check(schema) > - for m in v.type.members: > - m.check_clash(vseen) > + v.type.check_clash(schema, dict(seen)) > > > class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
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> > > --- > v10: rebase on new Variants.check_clash() > v9: new patch, split off from v8 7/17 > --- > scripts/qapi.py | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 4c56935..6d8c4c7 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -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) > + > def is_implicit(self): > # See QAPISchema._make_implicit_object_type() > return self.name[0] == ':' > @@ -1062,12 +1064,7 @@ 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 > - v.type.check(schema) > - for m in v.type.members: > - m.check_clash(vseen) > + v.type.check_clash(schema, dict(seen)) > > > class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
On 11/09/2015 06:00 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) > > This assertion is lost. > >> - 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) Directly lost, but indirectly still present. The new code is calling QAPISchemaObjectType.check_clash(), which won't exist unless self.base is a QAPISchemaObjectType. Folding the assert into the refactored function makes no sense (the condition isinstance(self, QAPISchemaObjectType) would always be true), and leaving the assert prior to calling self.base.check_clash() adds no real protection against programming bugs. >> @@ -1062,12 +1064,7 @@ 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) > > This assertion is lost. > >> - assert not v.type.variants # not implemented >> - v.type.check(schema) >> - for m in v.type.members: >> - m.check_clash(vseen) >> + v.type.check_clash(schema, dict(seen)) Same explanation.
Eric Blake <eblake@redhat.com> writes: > On 11/09/2015 06:00 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) >> >> This assertion is lost. >> >>> - 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) > > Directly lost, but indirectly still present. The new code is calling > QAPISchemaObjectType.check_clash(), which won't exist unless self.base > is a QAPISchemaObjectType. or a QAPISchemaObjectTypeMember, or a QAPISchemaObjectVariants, or whatever else acquires the method in the future. > Folding the assert into the refactored > function makes no sense (the condition isinstance(self, > QAPISchemaObjectType) would always be true), Correct. > and leaving the assert > prior to calling self.base.check_clash() adds no real protection against > programming bugs. Maybe, but the isinstance(self.base, QAPISchemaObjectType) will come right back anyway when we move the "'base' for FOO cannot use BAR type" check from the old semantic analysis into the check methods. Until then, it makes sense at least as a place holder. >>> @@ -1062,12 +1064,7 @@ 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) >> >> This assertion is lost. >> >>> - assert not v.type.variants # not implemented >>> - v.type.check(schema) >>> - for m in v.type.members: >>> - m.check_clash(vseen) >>> + v.type.check_clash(schema, dict(seen)) > > Same explanation.
On 11/09/2015 12:11 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 11/09/2015 06:00 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) >>> >>> This assertion is lost. >>> >> >> Directly lost, but indirectly still present. The new code is calling >> QAPISchemaObjectType.check_clash(), which won't exist unless self.base >> is a QAPISchemaObjectType. > > or a QAPISchemaObjectTypeMember, or a QAPISchemaObjectVariants, or > whatever else acquires the method in the future. > > > Maybe, but the isinstance(self.base, QAPISchemaObjectType) will come > right back anyway when we move the "'base' for FOO cannot use BAR type" > check from the old semantic analysis into the check methods. Until > then, it makes sense at least as a place holder. Good point, I'll add it back in.
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.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 4c56935..6d8c4c7 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -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) + 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,12 +1064,7 @@ 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 - v.type.check(schema) - for m in v.type.members: - m.check_clash(vseen) + v.type.check_clash(schema, dict(seen)) class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
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> --- v10: rebase on new Variants.check_clash() v9: new patch, split off from v8 7/17 --- scripts/qapi.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)