Re: [Qemu-devel] [PATCH v6 02/12] 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 QAPISchemaObjectType,
> and use that function where needed.  (Done at the ObjectType
> level, since we already know all builtins and arrays are
> implicit, no commands or events are implicit, and we don't
> have any differences in generated code for regular vs.
> implicit enums.)
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> 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 |  3 ++-
>  scripts/qapi-visit.py |  3 ++-
>  scripts/qapi.py       | 10 +++++++---
>  3 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index c5b71b0..6bac5b3 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -234,7 +234,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>          self._btin = None
>
>      def visit_predicate(self, entity):
> -        return not isinstance(entity, QAPISchemaObjectType) or entity.info
> +        return not (isinstance(entity, QAPISchemaObjectType) and
> +                    entity.is_implicit())

Aha, now the left hand side becomes necessary to guard the
.is_implicit().  It stays superfluous if you make .is_implicit() a
QAPISchemaEntity method.

>
>      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 0f47614..2957c85 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -334,7 +334,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>          self._btin = None
>
>      def visit_predicate(self, entity):
> -        return not isinstance(entity, QAPISchemaObjectType) or entity.info
> +        return not (isinstance(entity, QAPISchemaObjectType) and
> +                    entity.is_implicit())
>
>      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 7d359c8..8123ab3 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -970,12 +970,15 @@ class QAPISchemaObjectType(QAPISchemaType):
>              self.variants.check(schema, members, seen)
>          self.members = members
>
> +    def is_implicit(self):
> +        return self.name[0] == ':'
> +

If this test is here to stay, perhaps add a comment pointing to
_make_implicit_object_type().

>      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):
> @@ -1043,7 +1046,8 @@ 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 isinstance(self.type,
> +                      QAPISchemaObjectType) and self.type.is_implicit():

I figure you break this line in the argument list to avoid a backslash
for line continuation.  I know PEP8 doesn't like them, but I like line
breaks away from top level operators even less.  I feel it should be
broken after the and operator, even though that'll require wrapping the
condition in parenthesis.

>              assert len(self.type.members) == 1
>              assert not self.type.variants
>              return self.type.members[0].type