[RFC] qapi: Use callback to determine visit filtering

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 <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---

>>   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).

 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(-)

Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>
>>>   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?

On 10/01/2015 12:12 AM, Markus Armbruster wrote:

>>
>>> 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.

Returning True to visit and False to skip was easier to rationalize
about, so I've made that change in my local tree.


> 
> 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?

Oh, nice idea. Then we don't even have to return it during visit_begin()
- we just blindly call it.  Will work that into my local tree, and it
will be ready when I post (the next subset) of v6.