Re: [Qemu-devel] [PATCH v14 01/19] qapi: Consolidate object visitors

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>

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