| Message ID | 1442872682-6523-12-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@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 <eblake@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.
On 09/28/2015 06:43 AM, Markus Armbruster wrote: > Eric Blake <eblake@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. Especially since the current implementation crashes hard when trying to report an error with info=None. > >> 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. Fair enough; the proposal to separate it into its own patch, so we can then discard or easily revert it, sounds like the right approach. >> 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. Yeah, I thought about that. All builtin types are implicit, all array types are implicit, no commands or events are implicit, and we didn't make any different generated output based on whether enums were explicit or implicit, so that leaves just objects. >> +++ 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(): Another idea would be change the signature from: def visit_object_type(self (QAPISchemaVisitor), name (str), info (dict), base (QEMUSchemaObjectType), members (list of QEMUSchemaObjectTypeMember), variants (QAPISchemaObjectTypeVariants), implicit (bool)) to: def visit_object_type(self, object (QEMUSchemaObjectType)) and let callers dereference object.name, object.info, object.base, object.members (or object.local_members), object.variants, and object.is_implicit() as they see fit. (In fact, in one of my later patches, I already wished I had access to the actual QEMUSchemaObjectType object rather than its breakdown of parts to begin with, and ended up doing a schema.lookupType(name) just to get back to that piece of information). > > * 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. Maybe. Would also make implementing is_implicit() easy (which type did I instantiate) rather than hacky (does name start with ':'). > > 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. I'm still thinking about this one. > > 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. Yes, I'll go along with a split somewhere along these lines before reposting this patch for v6, although I'm going to have to sleep on it before deciding how to clean up the filtering.
Eric Blake <eblake@redhat.com> writes: > On 09/28/2015 06:43 AM, Markus Armbruster wrote: >> Eric Blake <eblake@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. > > Especially since the current implementation crashes hard when trying to > report an error with info=None. > >> >>> 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. > > Fair enough; the proposal to separate it into its own patch, so we can > then discard or easily revert it, sounds like the right approach. [...] > So far, we've used the '_' prefix only for instance variables that are > clearly internal. Mostly for stuff flowing from __init__() to check(). I'd rather not rename it at all now. Stick to our present use of the '_' prefix. >>> 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. > > Yeah, I thought about that. All builtin types are implicit, all array > types are implicit, no commands or events are implicit, and we didn't > make any different generated output based on whether enums were explicit > or implicit, so that leaves just objects. > >>> +++ 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(): > > Another idea would be change the signature from: > > def visit_object_type(self (QAPISchemaVisitor), name (str), > info (dict), base (QEMUSchemaObjectType), > members (list of QEMUSchemaObjectTypeMember), > variants (QAPISchemaObjectTypeVariants), > implicit (bool)) > > to: > > def visit_object_type(self, object (QEMUSchemaObjectType)) > > and let callers dereference object.name, object.info, object.base, > object.members (or object.local_members), object.variants, and > object.is_implicit() as they see fit. (In fact, in one of my later > patches, I already wished I had access to the actual > QEMUSchemaObjectType object rather than its breakdown of parts to begin > with, and ended up doing a schema.lookupType(name) just to get back to > that piece of information). If you apply this idea across the board, all the visit_FOO() take exactly two parameters, self and a FOO. Feels a bit like declaring bankruptcy on the visitor pattern... You start to wonder why we need a separate visit_FOO(). Nevertheless, I've felt the temptation myself. >> * 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. > > Maybe. Would also make implementing is_implicit() easy (which type did I > instantiate) rather than hacky (does name start with ':'). I don't mind the hacky bit, since you encapsulate it. >> Another option is generalizing QAPISchema's filter. How? We can always add an indirection: instead of parameterizing a fixed predicate (ignore and isinstance(entity, ignore)) with a type ignore, we could pass a predicate, i.e. a function mapping entity to bool. >> 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. > > I'm still thinking about this one. > >> >> 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. > > Yes, I'll go along with a split somewhere along these lines before > reposting this patch for v6, although I'm going to have to sleep on it > before deciding how to clean up the filtering. Sounds good.
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): + 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] == ':' + 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
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. 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. Signed-off-by: Eric Blake <eblake@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(-)