Re: [Qemu-devel] [PATCH v6 01/12] qapi: Use predicate callback to determine visit filtering

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2947 bytes --]

On 10/02/2015 12:47 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Previously, qapi-types and qapi-visit filtered out implicit
>> objects during visit_object_type() by using 'info' (works since
>> implicit objects do not [yet] have associated info); meanwhile
>> qapi-introspect filtered out all schema types on the first pass
>> by returning a python type from visit_begin(), which was then
>> used in QAPISchema.visit().  Rather than keeping these ad hoc
>> approaches, add a new visitor callback visit_predicate() which
>> returns False to skip a given entity, and which defaults to
>> True unless overridden.  Use the new mechanism to simplify all
>> three visitors that need filtering.  No change to the generated
>> code.
> 
> Let's call it visit_wanted().

Ah, that's nicer.

> 
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v6: new patch

>> +++ b/scripts/qapi-types.py
>> @@ -233,6 +233,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>>          self.decl = self._btin + self.decl
>>          self._btin = None
>>
>> +    def visit_predicate(self, entity):
>> +        return not isinstance(entity, QAPISchemaObjectType) or entity.info
>> +
> 
> This is the faithful translation.  But the left hand side is superfluous
> because non-types laways have info, isn't it?

Not yet.  3/12 adds info to arrays, but even then, intList still has
info of None.  And 4/12 adds info to implicit enums. Without the left
half, we choke hard for any implicit UnionKind enum type being omitted
as implicit.

However, I do think you're on to something - the only time name[0] ==
':' is for objects (implicit enums are NameKind, but since we don't have
anonymous unions, we won't have an enum starting with :; likewise,
arrays are NameList, but you can only have an array of a named type).
So if I hoist is_implicit() to the QAPISchemaEntity level, then it
should only ever return True for an object, and while _this_ patch has
to keep the left side, patch 2 gets the shorter conditional.

>> @@ -1304,10 +1307,10 @@ class QAPISchema(object):
>>              ent.check(self)
>>
>>      def visit(self, visitor):
>> -        ignore = visitor.visit_begin(self)
>> -        for name in sorted(self._entity_dict.keys()):
>> -            if not ignore or not isinstance(self._entity_dict[name], ignore):
>> -                self._entity_dict[name].visit(visitor)
>> +        visitor.visit_begin(self)
>> +        for (name, entity) in sorted(self._entity_dict.items()):
>> +            if visitor.visit_predicate(entity):
>> +                entity.visit(visitor)
>>          visitor.visit_end()
> 
> Much nicer than my ad hoc hackery!
> 

Yeah, I like how it turned out.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]