Re: [Qemu-devel] [PATCH v14 01/19] qapi: Consolidate object visitors

[Eric Blake]

On 04/13/2016 06:48 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Rather than having two separate visitor callbacks with items
>> already broken out, pass the actual QAPISchemaObjectType object
>> to the visitor.  This lets the visitor access things like
>> type.is_implicit() without needing another parameter, resolving
>> a TODO from previous patches.
>>
>> For convenience and consistency, the 'name' and 'info' parameters
>> are still provided, even though they are now redundant with
>> 'typ.name' and 'typ.info'.
>>
>> Signed-off-by: Eric Blake <address@hidden>
> 
> I've made you push this one back in the queue a couple of times, because
> there are pros and cons, and the work at hand didn't actually require
> the patch.  At some point we need to decide.  Perhaps that point is now.
> 
> The patch replaces two somewhat unclean "is implicit" tests by clean
> .is_implicit() calls.  Any other use of the change in this series?

I'm not seeing any other direct simplification in this series.  As a
quick test, I just rebased it to the end of this series with no merge
conflicts, and everything else still compiled and passed without it.
I'm less sure whether any of my later pending series depend on it.

> 
> Recap of pros and cons:
> 
> * The existing interface
> 
>       def visit_object_type(self, name, info, base, members, variants):
>       def visit_object_type_flat(self, name, info, members, variants):
> 
>   is explicit and narrow, but when you need more information, you have
>   to add parameters or functions.
> 
> * The new interface
> 
>      def visit_object_type(self, name, info, typ):
> 
>   avoids that, but now its users can access everything.
> 
> This patch touches only visiting of objects, because only for objects we
> have a TODO.  Should we change the other visit_ methods as well, for
> consistency?

I have a pending patch in subset F (last posted at v6) that adds a 'box'
parameter to visit_event and visit_command:
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg04397.html
If we change all the other visit_ methods for consistency, then those
methods would directly access command.box and event.box instead of
needing to add a separate parameter.

> 
>>
>> ---
>> v14: fix testsuite failures
>> [posted earlier as part of "easier unboxed visits/qapi implicit types"]
>> v6: new patch

>> +    def visit_object_type(self, name, info, typ):
>>          # Nothing to do for the special empty builtin
>>          if name == 'q_empty':
>>              return
>>          self.decl += gen_visit_members_decl(name)
>> -        self.defn += gen_visit_object_members(name, base, members, variants)
>> -        # TODO Worth changing the visitor signature, so we could
>> -        # directly use rather than repeat type.is_implicit()?
>> -        if not name.startswith('q_'):
>> +        self.defn += gen_visit_object_members(name, typ.base,
>> +                                              typ.local_members, 
>> typ.variants)
> Line gets a bit long.  Hanging indent?  Or change
> gen_visit_object_members() to take the type?

gen_visit_object_members() taking the type seems reasonable.

> 
>> +        if not typ.is_implicit():
>>              # only explicit types need an allocating visit
>>              self.decl += gen_visit_decl(name)
>> -            self.defn += gen_visit_object(name, base, members, variants)
>> +            self.defn += gen_visit_object(name, typ.base, typ.local_members,
>> +                                          typ.variants)

but gen_visit_object() still has to take the explosion of data because
it is shared by alternates, where we have to special-case .local_members.

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

signature.asc
Description: OpenPGP digital signature