Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 11/09/2015 06:00 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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 <address@hidden>
>>>
>>> ---
>
>>> @@ -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.