Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com> > --- > 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.
