[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of imp
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type |
Date: |
Fri, 02 Oct 2015 16:12:08 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.
- Re: [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names, (continued)
[Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member, Eric Blake, 2015/10/08
[Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type, Eric Blake, 2015/10/08
- Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type, Markus Armbruster, 2015/10/08
- Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type, Eric Blake, 2015/10/08
- Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type, Eric Blake, 2015/10/08
- Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type, Markus Armbruster, 2015/10/08
- Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type, Eric Blake, 2015/10/08
- Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type, Markus Armbruster, 2015/10/08
- Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type, Eric Blake, 2015/10/08
- Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type, Markus Armbruster, 2015/10/08
Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type, Eric Blake, 2015/10/08
[Qemu-devel] [PATCH v6 05/12] qapi: Track location that created an implicit type, Eric Blake, 2015/10/08