[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 11/46] qapi: Don't use info as witness of imp
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v5 11/46] qapi: Don't use info as witness of implicit object type |
Date: |
Mon, 28 Sep 2015 14:43:19 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.
> 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.
When you create an stable interface for use in other programs,
religiously hiding instance variables behind accessor methods can pay.
But in a purely internal interface like this one, I don't see the point.
If we run into a case where we want to use a QAPISchemaEntity's info, I
want to write .info and be done with it. If we rename it to _info now,
we'll rename it back then.
So far, we've used the '_' prefix only for instance variables that are
clearly internal. Mostly for stuff flowing from __init__() to check().
> Signed-off-by: Eric Blake <address@hidden>
> ---
> scripts/qapi-types.py | 4 ++--
> scripts/qapi-visit.py | 4 ++--
> scripts/qapi.py | 33 +++++++++++++++++++--------------
> tests/qapi-schema/test-qapi.py | 2 +-
> 4 files changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index b292682..aa25e03 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -253,8 +253,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> self.decl += gen_array(name, element_type)
> self._gen_type_cleanup(name)
>
> - def visit_object_type(self, name, info, base, members, variants):
> - if info:
> + def visit_object_type(self, name, info, base, members, variants,
> implicit):
This is now right at the PEP8 line length limit, and the number of
parameters is getting unwieldy, too. Hmm.
> + if not implicit:
> self._fwdecl += gen_fwd_object_or_array(name)
> if variants:
> assert not members # not implemented
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 1f287ba..62a47fa 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -348,8 +348,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
> self.decl += decl
> self.defn += defn
>
> - def visit_object_type(self, name, info, base, members, variants):
> - if info:
> + def visit_object_type(self, name, info, base, members, variants,
> implicit):
> + if not implicit:
> self.decl += gen_visit_decl(name)
> if variants:
> assert not members # not implemented
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 7275daa..1dc7641 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -786,7 +786,7 @@ class QAPISchemaEntity(object):
> def __init__(self, name, info):
> assert isinstance(name, str)
> self.name = name
> - self.info = info
> + self._info = info
>
> def c_name(self):
> return c_name(self.name)
> @@ -814,7 +814,7 @@ class QAPISchemaVisitor(object):
> def visit_array_type(self, name, info, element_type):
> pass
>
> - def visit_object_type(self, name, info, base, members, variants):
> + def visit_object_type(self, name, info, base, members, variants,
> implicit):
> pass
>
> def visit_object_type_flat(self, name, info, members, variants):
> @@ -877,7 +877,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
> return self._json_type_name
>
> def visit(self, visitor):
> - visitor.visit_builtin_type(self.name, self.info, self.json_type())
> + visitor.visit_builtin_type(self.name, self._info, self.json_type())
>
>
> class QAPISchemaEnumType(QAPISchemaType):
> @@ -903,7 +903,7 @@ class QAPISchemaEnumType(QAPISchemaType):
> return 'string'
>
> def visit(self, visitor):
> - visitor.visit_enum_type(self.name, self.info,
> + visitor.visit_enum_type(self.name, self._info,
> self.values, self.prefix)
>
>
> @@ -922,7 +922,7 @@ class QAPISchemaArrayType(QAPISchemaType):
> return 'array'
>
> def visit(self, visitor):
> - visitor.visit_array_type(self.name, self.info, self.element_type)
> + visitor.visit_array_type(self.name, self._info, self.element_type)
>
>
> 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.
> def c_name(self):
> - assert self.info
> + assert not self.is_implicit()
> return QAPISchemaType.c_name(self)
>
> def c_type(self, is_param=False):
> - assert self.info
> + assert not self.is_implicit()
> return QAPISchemaType.c_type(self)
>
> def json_type(self):
> return 'object'
>
> def visit(self, visitor):
> - visitor.visit_object_type(self.name, self.info,
> - self.base, self.local_members,
> self.variants)
> - visitor.visit_object_type_flat(self.name, self.info,
> + visitor.visit_object_type(self.name, self._info,
> + self.base, self.local_members,
> self.variants,
> + self.is_implicit())
> + visitor.visit_object_type_flat(self.name, self._info,
> self.members, self.variants)
>
>
> @@ -1034,7 +1038,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():
> assert len(self.type.members) == 1
> assert not self.type.variants
> return self.type.members[0].type
> @@ -1055,7 +1060,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
> return 'value'
>
> def visit(self, visitor):
> - visitor.visit_alternate_type(self.name, self.info, self.variants)
> + visitor.visit_alternate_type(self.name, self._info, self.variants)
>
>
> class QAPISchemaCommand(QAPISchemaEntity):
> @@ -1080,7 +1085,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
> assert isinstance(self.ret_type, QAPISchemaType)
>
> def visit(self, visitor):
> - visitor.visit_command(self.name, self.info,
> + visitor.visit_command(self.name, self._info,
> self.arg_type, self.ret_type,
> self.gen, self.success_response)
>
> @@ -1099,7 +1104,7 @@ class QAPISchemaEvent(QAPISchemaEntity):
> assert not self.arg_type.variants # not implemented
>
> def visit(self, visitor):
> - visitor.visit_event(self.name, self.info, self.arg_type)
> + visitor.visit_event(self.name, self._info, self.arg_type)
>
>
> class QAPISchema(object):
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 649677e..f2cce64 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ 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():
* 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.
Another option is generalizing QAPISchema's filter. How?
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.
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.
- Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call, (continued)
- [Qemu-devel] [PATCH v5 08/46] qapi: Reuse code for flat union base validation, Eric Blake, 2015/09/21
- [Qemu-devel] [PATCH v5 09/46] qapi: Use consistent generated code patterns, Eric Blake, 2015/09/21
- [Qemu-devel] [PATCH v5 10/46] qapi: Merge generation of per-member visits, Eric Blake, 2015/09/21
- [Qemu-devel] [PATCH v5 11/46] qapi: Don't use info as witness of implicit object type, Eric Blake, 2015/09/21
- Re: [Qemu-devel] [PATCH v5 11/46] qapi: Don't use info as witness of implicit object type,
Markus Armbruster <=
- [Qemu-devel] [PATCH v5 12/46] qapi: Track location that created an implicit type, Eric Blake, 2015/09/21
- [Qemu-devel] [PATCH v5 14/46] qapi: Detect collisions in C member names, Eric Blake, 2015/09/21
- [Qemu-devel] [PATCH v5 16/46] qapi: Detect base class loops, Eric Blake, 2015/09/21
- [Qemu-devel] [PATCH v5 15/46] qapi: Defer duplicate member checks to schema check(), Eric Blake, 2015/09/21
- [Qemu-devel] [PATCH v5 13/46] qapi: Track owner of each object member, Eric Blake, 2015/09/21
- [Qemu-devel] [PATCH v5 17/46] qapi: Provide nicer array names in introspection, Eric Blake, 2015/09/21
- [Qemu-devel] [PATCH v5 18/46] qapi-introspect: Guarantee particular sorting, Eric Blake, 2015/09/21