Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type

[Eric Blake]

On 10/02/2015 01:02 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> A future patch will enable error reporting from the various
>> QAPISchema*.check() methods.  But to report an error related
>> to 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.
>>
>> Instead, add an is_implicit() method to QAPISchemaObjectType,
>> and use that function where needed.  (Done at the ObjectType
>> level, since we already know all builtins and arrays are
>> implicit, no commands or events are implicit, and we don't
>> have any differences in generated code for regular vs.
>> implicit enums.)

>> +++ b/scripts/qapi-types.py
>> @@ -234,7 +234,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>>          self._btin = None
>>
>>      def visit_predicate(self, entity):
>> -        return not isinstance(entity, QAPISchemaObjectType) or entity.info
>> +        return not (isinstance(entity, QAPISchemaObjectType) and
>> +                    entity.is_implicit())
> 
> Aha, now the left hand side becomes necessary to guard the
> .is_implicit().  It stays superfluous if you make .is_implicit() a
> QAPISchemaEntity method.

See discussion on 1/12 - actually, it _becomes_ superfluous in this
patch once we make it a QAPISchemaEntity method.

>> +++ b/scripts/qapi.py
>> @@ -970,12 +970,15 @@ class QAPISchemaObjectType(QAPISchemaType):
>>              self.variants.check(schema, members, seen)
>>          self.members = members
>>
>> +    def is_implicit(self):
>> +        return self.name[0] == ':'

Actually, this only works for implicit objects.  Implicit enums instead
have self.name[-4:] == 'Kind'.  But qapi-types cares about implicit
objects only.  So if I hoist this, I may need something like:

def is_implicit(self, type=None):
    if type and not isinstance(self, type):
        return Fals
    if isinstance(self, QAPISchemaObjectType):
        return self.name[0] == ':'
    if isinstance(self, QAPISchemaEnumType):
        return self.name[-4:] == 'Kind'
    return False

where qapi-types would call entity.is_implicit(QAPISchemaObjectType).

>> +
> 
> If this test is here to stay, perhaps add a comment pointing to
> _make_implicit_object_type().

Indeed.


>> @@ -1043,7 +1046,8 @@ class 
>> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>>      # This function exists to support ugly simple union special cases
>>      # TODO get rid of them, and drop the function
>>      def simple_union_type(self):
>> -        if isinstance(self.type, QAPISchemaObjectType) and not 
>> self.type.info:
>> +        if isinstance(self.type,
>> +                      QAPISchemaObjectType) and self.type.is_implicit():
> 
> I figure you break this line in the argument list to avoid a backslash
> for line continuation.  I know PEP8 doesn't like them, but I like line
> breaks away from top level operators even less.  I feel it should be
> broken after the and operator, even though that'll require wrapping the
> condition in parenthesis.

Sadly, this form causes pep8 to complain about continuation at the same
indentation as the body:

if (cond1 and
    cond2):
    body

But it seems that pep8 and pylint don't mind the backslash in:

if cond1 and \
   cond2:
    body

where the indentation is okay.  I guess trying to avoid the \ is not
worth it, if the tools don't complain about it, and that this was a case
of me prematurely guessing (incorrectly) about what the tools don't like.


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

signature.asc
Description: OpenPGP digital signature