On 09/28/2015 06:43 AM, Markus Armbruster wrote: > 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.
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 +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature