Re: [Qemu-devel] [PATCH v5 11/46] qapi: Don't use info as witness of implicit object type

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 09/28/2015 06:43 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> A future patch will enable error reporting from the various
>>> check() methods.  But to report an error on an implicit type,
>>> we'll need to associate a location with the type (the same
>>> location as the top-level entity that is causing the creation
>>> of the implicit type), and once we do that, keying off of
>>> whether foo.info exists is no longer a viable way to determine
>>> if foo is an implicit type.
>> 
>> Ensuring error messages are good even for implicit types could be hard.
>> But pretty much anything's better than error messages without location
>> information.
>
> Especially since the current implementation crashes hard when trying to
> report an error with info=None.
>
>> 
>>> Rename the info member to _info (so that sub-classes can still
>>> use it, but external code should not), add an is_implicit()
>>> method to QAPISchemaObjectType, and adjust the visitor to pass
>>> another parameter about whether the type is implicit.
>> 
>> I have doubts on the rename.
>
> Fair enough; the proposal to separate it into its own patch, so we can
> then discard or easily revert it, sounds like the right approach.
[...]
> So far, we've used the '_' prefix only for instance variables that are
> clearly internal.  Mostly for stuff flowing from __init__() to check().

I'd rather not rename it at all now.  Stick to our present use of the
'_' prefix.

>>>  class QAPISchemaObjectType(QAPISchemaType):
>>> @@ -961,21 +961,25 @@ class QAPISchemaObjectType(QAPISchemaType):
>>>              self.variants.check(schema, members, seen)
>>>          self.members = members
>>>
>>> +    def is_implicit(self):
>>> +        return self.name[0] == ':'
>>> +
>> 
>> The predicate could be defined on any QAPISchemaType, or even any
>> QAPISchemaEntity, but right now we only ever want to test it for
>> objects.  Okay.
>
> Yeah, I thought about that.  All builtin types are implicit, all array
> types are implicit, no commands or events are implicit, and we didn't
> make any different generated output based on whether enums were explicit
> or implicit, so that leaves just objects.
>
>>> +++ b/tests/qapi-schema/test-qapi.py
>>> @@ -22,7 +22,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>>>          if prefix:
>>>              print '    prefix %s' % prefix
>>>
>>> -    def visit_object_type(self, name, info, base, members, variants):
>>> +    def visit_object_type(self, name, info, base, members, variants, 
>>> implicit):
>>>          print 'object %s' % name
>>>          if base:
>>>              print '    base %s' % base.name
>> 
>> Three of our visitors implement visit_object_type():
>
> Another idea would be change the signature from:
>
> def visit_object_type(self (QAPISchemaVisitor), name (str),
>                       info (dict), base (QEMUSchemaObjectType),
>                       members (list of QEMUSchemaObjectTypeMember),
>                       variants (QAPISchemaObjectTypeVariants),
>                       implicit (bool))
>
> to:
>
> def visit_object_type(self, object (QEMUSchemaObjectType))
>
> and let callers dereference object.name, object.info, object.base,
> object.members (or object.local_members), object.variants, and
> object.is_implicit() as they see fit. (In fact, in one of my later
> patches, I already wished I had access to the actual
> QEMUSchemaObjectType object rather than its breakdown of parts to begin
> with, and ended up doing a schema.lookupType(name) just to get back to
> that piece of information).

If you apply this idea across the board, all the visit_FOO() take
exactly two parameters, self and a FOO.  Feels a bit like declaring
bankruptcy on the visitor pattern...  You start to wonder why we need a
separate visit_FOO().

Nevertheless, I've felt the temptation myself.

>> * test-qapi.py doesn't care about implicit (implicitness is obvious
>>   enough from the name here).
>> 
>> * qapi-types.py and qapi-visit.py ignore implicit object types.  Hmm.
>> 
>>   qapi-introspect.py has a similar need: it wants to ignore *all* types.
>>   Two ways to ignore entities seem one too many.  Preexisting, but your
>>   patch makes it stand out a bit more.
>> 
>>   Could we reuse the existing mechanism somehow (and keep method
>>   visit_object_type() simple)?
>> 
>>   To reuse it without changes, we'd have to make implicit object types a
>>   separate class, so that QAPISchema.visit()'s isinstance() test can be
>>   put to work.
>
> Maybe. Would also make implementing is_implicit() easy (which type did I
> instantiate) rather than hacky (does name start with ':').

I don't mind the hacky bit, since you encapsulate it.

>>   Another option is generalizing QAPISchema's filter.  How?

We can always add an indirection: instead of parameterizing a fixed
predicate (ignore and isinstance(entity, ignore)) with a type ignore, we
could pass a predicate, i.e. a function mapping entity to bool.

>>   A third option is to abandon QAPISchema's filter, and make
>>   qapi-introspect.py filter in the visitor methods, just like we filter
>>   implicit objects.
>
> I'm still thinking about this one.
>
>> 
>> Patch could be split into
>> 
>> A. Encapsulate the "is implicit" predicate in a method, i.e. replace
>>    not o.info by o.is_implicit().
>> 
>> B. Clean up how we filter out implicit objects.  May better go before A,
>>    not sure.
>> 
>> C. Rename .info to ._info.  Not sure we even want this part.
>
> Yes, I'll go along with a split somewhere along these lines before
> reposting this patch for v6, although I'm going to have to sleep on it
> before deciding how to clean up the filtering.

Sounds good.