| Message ID | 1460131992-32278-2-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Rather than having two separate visitor callbacks with items > already broken out, pass the actual QAPISchemaObjectType object > to the visitor. This lets the visitor access things like > type.is_implicit() without needing another parameter, resolving > a TODO from previous patches. > > For convenience and consistency, the 'name' and 'info' parameters > are still provided, even though they are now redundant with > 'typ.name' and 'typ.info'. > > Signed-off-by: Eric Blake <eblake@redhat.com> I've made you push this one back in the queue a couple of times, because there are pros and cons, and the work at hand didn't actually require the patch. At some point we need to decide. Perhaps that point is now. The patch replaces two somewhat unclean "is implicit" tests by clean .is_implicit() calls. Any other use of the change in this series? Recap of pros and cons: * The existing interface def visit_object_type(self, name, info, base, members, variants): def visit_object_type_flat(self, name, info, members, variants): is explicit and narrow, but when you need more information, you have to add parameters or functions. * The new interface def visit_object_type(self, name, info, typ): avoids that, but now its users can access everything. This patch touches only visiting of objects, because only for objects we have a TODO. Should we change the other visit_ methods as well, for consistency? > > --- > v14: fix testsuite failures > [posted earlier as part of "easier unboxed visits/qapi implicit types"] > v6: new patch > --- > scripts/qapi.py | 10 ++-------- > scripts/qapi-introspect.py | 10 +++++----- > scripts/qapi-types.py | 13 ++++++------- > scripts/qapi-visit.py | 12 ++++++------ > tests/qapi-schema/test-qapi.py | 10 +++++----- > 5 files changed, 24 insertions(+), 31 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index b13ae47..4dde43a 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -808,10 +808,7 @@ class QAPISchemaVisitor(object): > def visit_array_type(self, name, info, element_type): > pass > > - def visit_object_type(self, name, info, base, members, variants): > - pass > - > - def visit_object_type_flat(self, name, info, members, variants): > + def visit_object_type(self, name, info, typ): x> pass > > def visit_alternate_type(self, name, info, variants): > @@ -1005,10 +1002,7 @@ class QAPISchemaObjectType(QAPISchemaType): > 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, > - self.members, self.variants) > + visitor.visit_object_type(self.name, self.info, self) > > > class QAPISchemaMember(object): > diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py > index e0f926b..474eafd 100644 > --- a/scripts/qapi-introspect.py > +++ b/scripts/qapi-introspect.py > @@ -141,11 +141,11 @@ const char %(c_name)s[] = %(c_string)s; > element = self._use_type(element_type) > self._gen_json('[' + element + ']', 'array', {'element-type': element}) > > - def visit_object_type_flat(self, name, info, members, variants): > - obj = {'members': [self._gen_member(m) for m in members]} > - if variants: > - obj.update(self._gen_variants(variants.tag_member.name, > - variants.variants)) > + def visit_object_type(self, name, info, typ): > + obj = {'members': [self._gen_member(m) for m in typ.members]} > + if typ.variants: > + obj.update(self._gen_variants(typ.variants.tag_member.name, > + typ.variants.variants)) > self._gen_json(name, 'object', obj) > > def visit_alternate_type(self, name, info, variants): > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 437cf6c..60de4b6 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -220,17 +220,16 @@ 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): > + def visit_object_type(self, name, info, typ): > # Nothing to do for the special empty builtin > if name == 'q_empty': > return > self._fwdecl += gen_fwd_object_or_array(name) > - self.decl += gen_object(name, base, members, variants) > - if base and not base.is_implicit(): > - self.decl += gen_upcast(name, base) > - # TODO Worth changing the visitor signature, so we could > - # directly use rather than repeat type.is_implicit()? > - if not name.startswith('q_'): > + self.decl += gen_object(name, typ.base, typ.local_members, > + typ.variants) > + if typ.base and not typ.base.is_implicit(): > + self.decl += gen_upcast(name, typ.base) > + if not typ.is_implicit(): > # implicit types won't be directly allocated/freed > self._gen_type_cleanup(name) > > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 31d2330..dc8b39c 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -289,18 +289,18 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): > self.decl += decl > self.defn += defn > > - def visit_object_type(self, name, info, base, members, variants): > + def visit_object_type(self, name, info, typ): > # Nothing to do for the special empty builtin > if name == 'q_empty': > return > self.decl += gen_visit_members_decl(name) > - self.defn += gen_visit_object_members(name, base, members, variants) > - # TODO Worth changing the visitor signature, so we could > - # directly use rather than repeat type.is_implicit()? > - if not name.startswith('q_'): > + self.defn += gen_visit_object_members(name, typ.base, > + typ.local_members, typ.variants) Line gets a bit long. Hanging indent? Or change gen_visit_object_members() to take the type? > + if not typ.is_implicit(): > # only explicit types need an allocating visit > self.decl += gen_visit_decl(name) > - self.defn += gen_visit_object(name, base, members, variants) > + self.defn += gen_visit_object(name, typ.base, typ.local_members, > + typ.variants) > > def visit_alternate_type(self, name, info, variants): > self.decl += gen_visit_decl(name) > diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py > index 649677e..ccd1704 100644 > --- a/tests/qapi-schema/test-qapi.py > +++ b/tests/qapi-schema/test-qapi.py > @@ -22,14 +22,14 @@ 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, typ): > print 'object %s' % name > - if base: > - print ' base %s' % base.name > - for m in members: > + if typ.base: > + print ' base %s' % typ.base.name > + for m in typ.local_members: > print ' member %s: %s optional=%s' % \ > (m.name, m.type.name, m.optional) > - self._print_variants(variants) > + self._print_variants(typ.variants) > > def visit_alternate_type(self, name, info, variants): > print 'alternate %s' % name
On 04/13/2016 06:48 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Rather than having two separate visitor callbacks with items >> already broken out, pass the actual QAPISchemaObjectType object >> to the visitor. This lets the visitor access things like >> type.is_implicit() without needing another parameter, resolving >> a TODO from previous patches. >> >> For convenience and consistency, the 'name' and 'info' parameters >> are still provided, even though they are now redundant with >> 'typ.name' and 'typ.info'. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > I've made you push this one back in the queue a couple of times, because > there are pros and cons, and the work at hand didn't actually require > the patch. At some point we need to decide. Perhaps that point is now. > > The patch replaces two somewhat unclean "is implicit" tests by clean > .is_implicit() calls. Any other use of the change in this series? I'm not seeing any other direct simplification in this series. As a quick test, I just rebased it to the end of this series with no merge conflicts, and everything else still compiled and passed without it. I'm less sure whether any of my later pending series depend on it. > > Recap of pros and cons: > > * The existing interface > > def visit_object_type(self, name, info, base, members, variants): > def visit_object_type_flat(self, name, info, members, variants): > > is explicit and narrow, but when you need more information, you have > to add parameters or functions. > > * The new interface > > def visit_object_type(self, name, info, typ): > > avoids that, but now its users can access everything. > > This patch touches only visiting of objects, because only for objects we > have a TODO. Should we change the other visit_ methods as well, for > consistency? I have a pending patch in subset F (last posted at v6) that adds a 'box' parameter to visit_event and visit_command: https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg04397.html If we change all the other visit_ methods for consistency, then those methods would directly access command.box and event.box instead of needing to add a separate parameter. > >> >> --- >> v14: fix testsuite failures >> [posted earlier as part of "easier unboxed visits/qapi implicit types"] >> v6: new patch >> + def visit_object_type(self, name, info, typ): >> # Nothing to do for the special empty builtin >> if name == 'q_empty': >> return >> self.decl += gen_visit_members_decl(name) >> - self.defn += gen_visit_object_members(name, base, members, variants) >> - # TODO Worth changing the visitor signature, so we could >> - # directly use rather than repeat type.is_implicit()? >> - if not name.startswith('q_'): >> + self.defn += gen_visit_object_members(name, typ.base, >> + typ.local_members, typ.variants) > Line gets a bit long. Hanging indent? Or change > gen_visit_object_members() to take the type? gen_visit_object_members() taking the type seems reasonable. > >> + if not typ.is_implicit(): >> # only explicit types need an allocating visit >> self.decl += gen_visit_decl(name) >> - self.defn += gen_visit_object(name, base, members, variants) >> + self.defn += gen_visit_object(name, typ.base, typ.local_members, >> + typ.variants) but gen_visit_object() still has to take the explosion of data because it is shared by alternates, where we have to special-case .local_members.
Eric Blake <eblake@redhat.com> writes: > On 04/13/2016 06:48 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Rather than having two separate visitor callbacks with items >>> already broken out, pass the actual QAPISchemaObjectType object >>> to the visitor. This lets the visitor access things like >>> type.is_implicit() without needing another parameter, resolving >>> a TODO from previous patches. >>> >>> For convenience and consistency, the 'name' and 'info' parameters >>> are still provided, even though they are now redundant with >>> 'typ.name' and 'typ.info'. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> I've made you push this one back in the queue a couple of times, because >> there are pros and cons, and the work at hand didn't actually require >> the patch. At some point we need to decide. Perhaps that point is now. >> >> The patch replaces two somewhat unclean "is implicit" tests by clean >> .is_implicit() calls. Any other use of the change in this series? > > I'm not seeing any other direct simplification in this series. As a > quick test, I just rebased it to the end of this series with no merge > conflicts, and everything else still compiled and passed without it. > I'm less sure whether any of my later pending series depend on it. > >> >> Recap of pros and cons: >> >> * The existing interface >> >> def visit_object_type(self, name, info, base, members, variants): >> def visit_object_type_flat(self, name, info, members, variants): >> >> is explicit and narrow, but when you need more information, you have >> to add parameters or functions. >> >> * The new interface >> >> def visit_object_type(self, name, info, typ): >> >> avoids that, but now its users can access everything. >> >> This patch touches only visiting of objects, because only for objects we >> have a TODO. Should we change the other visit_ methods as well, for >> consistency? > > I have a pending patch in subset F (last posted at v6) that adds a 'box' > parameter to visit_event and visit_command: > https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg04397.html > If we change all the other visit_ methods for consistency, then those > methods would directly access command.box and event.box instead of > needing to add a separate parameter. Let's move this patch into subset F (should be the next series, as this one is E), and figure it out there.
diff --git a/scripts/qapi.py b/scripts/qapi.py index b13ae47..4dde43a 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -808,10 +808,7 @@ class QAPISchemaVisitor(object): def visit_array_type(self, name, info, element_type): pass - def visit_object_type(self, name, info, base, members, variants): - pass - - def visit_object_type_flat(self, name, info, members, variants): + def visit_object_type(self, name, info, typ): pass def visit_alternate_type(self, name, info, variants): @@ -1005,10 +1002,7 @@ class QAPISchemaObjectType(QAPISchemaType): 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, - self.members, self.variants) + visitor.visit_object_type(self.name, self.info, self) class QAPISchemaMember(object): diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py index e0f926b..474eafd 100644 --- a/scripts/qapi-introspect.py +++ b/scripts/qapi-introspect.py @@ -141,11 +141,11 @@ const char %(c_name)s[] = %(c_string)s; element = self._use_type(element_type) self._gen_json('[' + element + ']', 'array', {'element-type': element}) - def visit_object_type_flat(self, name, info, members, variants): - obj = {'members': [self._gen_member(m) for m in members]} - if variants: - obj.update(self._gen_variants(variants.tag_member.name, - variants.variants)) + def visit_object_type(self, name, info, typ): + obj = {'members': [self._gen_member(m) for m in typ.members]} + if typ.variants: + obj.update(self._gen_variants(typ.variants.tag_member.name, + typ.variants.variants)) self._gen_json(name, 'object', obj) def visit_alternate_type(self, name, info, variants): diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 437cf6c..60de4b6 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -220,17 +220,16 @@ 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): + def visit_object_type(self, name, info, typ): # Nothing to do for the special empty builtin if name == 'q_empty': return self._fwdecl += gen_fwd_object_or_array(name) - self.decl += gen_object(name, base, members, variants) - if base and not base.is_implicit(): - self.decl += gen_upcast(name, base) - # TODO Worth changing the visitor signature, so we could - # directly use rather than repeat type.is_implicit()? - if not name.startswith('q_'): + self.decl += gen_object(name, typ.base, typ.local_members, + typ.variants) + if typ.base and not typ.base.is_implicit(): + self.decl += gen_upcast(name, typ.base) + if not typ.is_implicit(): # implicit types won't be directly allocated/freed self._gen_type_cleanup(name) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 31d2330..dc8b39c 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -289,18 +289,18 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): self.decl += decl self.defn += defn - def visit_object_type(self, name, info, base, members, variants): + def visit_object_type(self, name, info, typ): # Nothing to do for the special empty builtin if name == 'q_empty': return self.decl += gen_visit_members_decl(name) - self.defn += gen_visit_object_members(name, base, members, variants) - # TODO Worth changing the visitor signature, so we could - # directly use rather than repeat type.is_implicit()? - if not name.startswith('q_'): + self.defn += gen_visit_object_members(name, typ.base, + typ.local_members, typ.variants) + if not typ.is_implicit(): # only explicit types need an allocating visit self.decl += gen_visit_decl(name) - self.defn += gen_visit_object(name, base, members, variants) + self.defn += gen_visit_object(name, typ.base, typ.local_members, + typ.variants) def visit_alternate_type(self, name, info, variants): self.decl += gen_visit_decl(name) diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index 649677e..ccd1704 100644 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -22,14 +22,14 @@ 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, typ): print 'object %s' % name - if base: - print ' base %s' % base.name - for m in members: + if typ.base: + print ' base %s' % typ.base.name + for m in typ.local_members: print ' member %s: %s optional=%s' % \ (m.name, m.type.name, m.optional) - self._print_variants(variants) + self._print_variants(typ.variants) def visit_alternate_type(self, name, info, variants): print 'alternate %s' % name
Rather than having two separate visitor callbacks with items already broken out, pass the actual QAPISchemaObjectType object to the visitor. This lets the visitor access things like type.is_implicit() without needing another parameter, resolving a TODO from previous patches. For convenience and consistency, the 'name' and 'info' parameters are still provided, even though they are now redundant with 'typ.name' and 'typ.info'. Signed-off-by: Eric Blake <eblake@redhat.com> --- v14: fix testsuite failures [posted earlier as part of "easier unboxed visits/qapi implicit types"] v6: new patch --- scripts/qapi.py | 10 ++-------- scripts/qapi-introspect.py | 10 +++++----- scripts/qapi-types.py | 13 ++++++------- scripts/qapi-visit.py | 12 ++++++------ tests/qapi-schema/test-qapi.py | 10 +++++----- 5 files changed, 24 insertions(+), 31 deletions(-)