Re: [Qemu-devel] [RFC PATCH] qapi: Use callback to determine visit filtering

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Previously, qapi-types and qapi-visit filtered an object visit
> to bypass implicit types by inspecting whether 'info' was passed
> in (since implicit objects do not [yet] have associated info);
> meanwhile qapi-introspect filtered by returning a python type
> from visit_begin() in order to exclude all type entities on the
> first pass.  Rather than keeping these ad hoc methods, rework
> the contract of visit_begin() to state that it may return a
> predicate function that returns True to ignore a given entity,
> and use that mechanism to simplify all three visitors.
>
> Suggested-by: Markus Armbruster <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>
>>>   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.
>
> Like this?  Generates the same code before and after the
> patch.  I'm open to suggestions on any way to make it more
> idiomatic python, althouth it at least passed pep8.  In
> particular, I'm wondering if the predicate should have its
> sense reversed (pass False to skip, True to visit).
>
> I'll probably drop the 'assert info' lines in the two
> visit_object_type() calls (I put it there to make sure the
> predicate was working).

Yes, please.

>  scripts/qapi-introspect.py |  5 ++++-
>  scripts/qapi-types.py      | 20 ++++++++++++--------
>  scripts/qapi-visit.py      | 18 +++++++++++-------
>  scripts/qapi.py            |  4 +++-
>  4 files changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index 7d39320..37b4306 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -54,7 +54,7 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
>          self._jsons = []
>          self._used_types = []
>          self._name_map = {}
> -        return QAPISchemaType   # don't visit types for now
> +        return self._visit_filter   # don't visit types for now
>
>      def visit_end(self):
>          # visit the types that are actually used
> @@ -82,6 +82,9 @@ const char %(c_name)s[] = %(c_string)s;
>          self._used_types = None
>          self._name_map = None
>
> +    def _visit_filter(self, entity):
> +        return isinstance(entity, QAPISchemaType)
> +
>      def _name(self, name):
>          if self._unmask:
>              return name
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index d405f8d..fbe74e1 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -213,12 +213,16 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>          self._fwdefn = None
>          self._btin = None
>
> +    def _visit_filter(self, entity):
> +        return isinstance(entity, QAPISchemaObjectType) and not entity.info
> +
>      def visit_begin(self, schema):
>          self.decl = ''
>          self.defn = ''
>          self._fwdecl = ''
>          self._fwdefn = ''
>          self._btin = guardstart('QAPI_TYPES_BUILTIN')
> +        return self._visit_filter    # ignore builtin types
>
>      def visit_end(self):
>          self.decl = self._fwdecl + self.decl
> @@ -254,14 +258,14 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>              self._gen_type_cleanup(name)
>
>      def visit_object_type(self, name, info, base, members, variants):
> -        if info:
> -            self._fwdecl += gen_fwd_object_or_array(name)
> -            if variants:
> -                assert not members      # not implemented
> -                self.decl += gen_union(name, base, variants)
> -            else:
> -                self.decl += gen_struct(name, base, members)
> -            self._gen_type_cleanup(name)
> +        assert info
> +        self._fwdecl += gen_fwd_object_or_array(name)
> +        if variants:
> +            assert not members      # not implemented
> +            self.decl += gen_union(name, base, variants)
> +        else:
> +            self.decl += gen_struct(name, base, members)
> +        self._gen_type_cleanup(name)
>
>      def visit_alternate_type(self, name, info, variants):
>          self._fwdecl += gen_fwd_object_or_array(name)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 4f97781..be1bba7 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -319,10 +319,14 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>          self.defn = None
>          self._btin = None
>
> +    def _visit_filter(self, entity):
> +        return isinstance(entity, QAPISchemaObjectType) and not entity.info
> +
>      def visit_begin(self, schema):
>          self.decl = ''
>          self.defn = ''
>          self._btin = guardstart('QAPI_VISIT_BUILTIN')
> +        return self._visit_filter    # ignore builtin types
>
>      def visit_end(self):
>          # To avoid header dependency hell, we always generate
> @@ -349,13 +353,13 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>              self.defn += defn
>
>      def visit_object_type(self, name, info, base, members, variants):
> -        if info:
> -            self.decl += gen_visit_decl(name)
> -            if variants:
> -                assert not members      # not implemented
> -                self.defn += gen_visit_union(name, base, variants)
> -            else:
> -                self.defn += gen_visit_struct(name, base, members)
> +        assert info
> +        self.decl += gen_visit_decl(name)
> +        if variants:
> +            assert not members      # not implemented
> +            self.defn += gen_visit_union(name, base, variants)
> +        else:
> +            self.defn += gen_visit_struct(name, base, members)
>
>      def visit_alternate_type(self, name, info, variants):
>          self.decl += gen_visit_decl(name)
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 7761b4b..0fadc05 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -805,6 +805,8 @@ class QAPISchemaEntity(object):
>
>
>  class QAPISchemaVisitor(object):
> +    # To ignore certain entities, return a predicate function such
> +    # that predicate(entity) returns True to ignore that entity
>      def visit_begin(self, schema):
>          pass
>
> @@ -1306,7 +1308,7 @@ class QAPISchema(object):
>      def visit(self, visitor):
>          ignore = visitor.visit_begin(self)
>          for name in sorted(self._entity_dict.keys()):
> -            if not ignore or not isinstance(self._entity_dict[name], ignore):
> +            if not ignore or not ignore(self._entity_dict[name]):
>                  self._entity_dict[name].visit(visitor)
>          visitor.visit_end()

I think this turned out rather nicely.

Can we go one step further?  Unconditionally call visitor.visit_filter()
here, define the pass-everything filter in QAPISchemaVisitor, override
it as needed.

Name it visit_filter_out() to make the sense of the return value
obvious?