Eric Blake <ebl...@redhat.com> writes: > 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. [...] > 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.
