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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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.

Let's move this patch into subset F (should be the next series, as this
one is E), and figure it out there.