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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

Do it the OO-way: QAPISchemaEntity.is_implicit() returns False.  Any
subclass that can have implicitly defined instances overrides it:
QAPISchemaObjectType.is_implicit() tests for ':' prefix,
QAPISchemaEnumType.is_implicit() tests for 'Kind' suffix (requires
outlawing it for user enums, if we don't do it already), and so forth.

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

Quoting PEP8:

    When the conditional part of an if -statement is long enough to
    require that it be written across multiple lines, it's worth noting
    that the combination of a two character keyword (i.e. if ), plus a
    single space, plus an opening parenthesis creates a natural 4-space
    indent for the subsequent lines of the multiline conditional. This
    can produce a visual conflict with the indented suite of code nested
    inside the if -statement, which would also naturally be indented to
    4 spaces. This PEP takes no explicit position on how (or whether) to
    further visually distinguish such conditional lines from the nested
    suite inside the if -statement. Acceptable options in this situation
    include, but are not limited to:

    # No extra indentation.
    if (this_is_one_thing and
        that_is_another_thing):
        do_something()

    # Add a comment, which will provide some distinction in editors
    # supporting syntax highlighting.
    if (this_is_one_thing and
        that_is_another_thing):
        # Since both conditions are true, we can frobnicate.
        do_something()

    # Add some extra indentation on the conditional continuation line.
    if (this_is_one_thing
            and that_is_another_thing):
        do_something()

The last one could make pep8 and pylint happy.