Re: [Qemu-devel] [PATCH v7 04/14] qapi: Don't use info as witness of implicit object type

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> A future patch will enable error reporting from the various
> QAPISchema*.check() methods.  But to report an error related
> to 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.
>
> Instead, add an is_implicit() method to QAPISchemaEntity, with
> an internal helper overridden for ObjectType and EnumType, and
> use that function where needed.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v7: declare at the Entity level, with an optional argument for
> filtering by sub-class
> v6: split 11/46 into pieces; don't rename _info yet; rework atop
> nicer filtering mechanism, including no need to change visitor
> signature
> ---
>  scripts/qapi-types.py |  2 +-
>  scripts/qapi-visit.py |  2 +-
>  scripts/qapi.py       | 24 +++++++++++++++++++++---
>  3 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2a29c6e..227ea5c 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -235,7 +235,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>
>      def visit_needed(self, entity):
>          # Visit everything except implicit objects
> -        return not isinstance(entity, QAPISchemaObjectType) or entity.info
> +        return not entity.is_implicit(QAPISchemaObjectType)

The alternative is something like

        return not (isinstance(entity, QAPISchemaObjectType) and
                    entity.is_implicit())

Trades a more verbose "is this an implicit object type" check for a
simpler is_implicit().  Shorter overall, and feels better to me.  But if
you feel strongly about your way of doing it, I can live with it.

>
>      def _gen_type_cleanup(self, name):
>          self.decl += gen_type_cleanup_decl(name)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 9fc79a7..56b8390 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -335,7 +335,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>
>      def visit_needed(self, entity):
>          # Visit everything except implicit objects
> -        return not isinstance(entity, QAPISchemaObjectType) or entity.info
> +        return not entity.is_implicit(QAPISchemaObjectType)
>
>      def visit_enum_type(self, name, info, values, prefix):
>          self.decl += gen_visit_decl(name, scalar=True)
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4573599..19cc78e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -801,6 +801,16 @@ class QAPISchemaEntity(object):
>      def check(self, schema):
>          pass
>
> +    # Return True if this entity is implicit
> +    # If @typ, only return True if entity is also an instance of that type
> +    def is_implicit(self, typ=None):
> +        if typ and not isinstance(self, typ):
> +            return False
> +        return self._is_implicit()
> +
> +    def _is_implicit(self):
> +        return not self.info
> +
>      def visit(self, visitor):
>          pass
>
> @@ -903,6 +913,10 @@ class QAPISchemaEnumType(QAPISchemaType):
>      def check(self, schema):
>          assert len(set(self.values)) == len(self.values)
>
> +    def _is_implicit(self):
> +        # See QAPISchema._make_implicit_enum_type()
> +        return self.name[-4:] == 'Kind'
> +
>      def c_type(self, is_param=False):
>          return c_name(self.name)
>

Might want to add a comment to _make_implicit_enum_type() that justifies
name + 'Kind' by pointing out add_name() rejects a 'Kind' suffix.

> @@ -973,12 +987,16 @@ class QAPISchemaObjectType(QAPISchemaType):
>              self.variants.check(schema, members, seen)
>          self.members = members
>
> +    def _is_implicit(self):
> +        # See QAPISchema._make_implicit_object_type()
> +        return self.name[0] == ':'
> +
>      def c_name(self):
> -        assert self.info
> +        assert not self.is_implicit()
>          return QAPISchemaType.c_name(self)
>
>      def c_type(self, is_param=False):
> -        assert self.info
> +        assert not self.is_implicit()
>          return QAPISchemaType.c_type(self)
>
>      def json_type(self):
> @@ -1046,7 +1064,7 @@ class 
> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      # This function exists to support ugly simple union special cases
>      # TODO get rid of them, and drop the function
>      def simple_union_type(self):
> -        if isinstance(self.type, QAPISchemaObjectType) and not 
> self.type.info:
> +        if self.type.is_implicit(QAPISchemaObjectType):
>              assert len(self.type.members) == 1
>              assert not self.type.variants
>              return self.type.members[0].type