Re: [Qemu-devel] [PATCH v13 10/14] qapi: Track enum values by QAPISchemaMember, not string

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Rather than using just an array of strings, make enum.values be
> an array of the new QAPISchemaMember type.  Creating an enum
> requires wrapping strings, and visiting an enum requires getting
> at the name of each value.  But using this type means we can
> share the existing code for C name clash detection (although the
> code is not yet active until a later commit removes the earlier
> ad hoc parser checks).  The ._pretty_owner() method needs to
> learn about one more implicit type name: the generated enum
> associated with a simple union.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v13: new patch
> ---
>  scripts/qapi.py | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 2748464..d1239c2 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -901,13 +901,17 @@ class QAPISchemaEnumType(QAPISchemaType):
>      def __init__(self, name, info, values, prefix):
>          QAPISchemaType.__init__(self, name, info)
>          for v in values:
> -            assert isinstance(v, str)
> +            assert isinstance(v, QAPISchemaMember)
> +            v.set_owner(name)
>          assert prefix is None or isinstance(prefix, str)
>          self.values = values
>          self.prefix = prefix
>
>      def check(self, schema):
> -        assert len(set(self.values)) == len(self.values)
> +        # Check for collisions on the generated C enum values
> +        seen = {}
> +        for v in self.values:
> +            v.check_clash(self.info, seen)

Is the comment worth its keep?  We have similar loops elsewhere, without
such a comment.

>
>      def is_implicit(self):
>          # See QAPISchema._make_implicit_enum_type()
> @@ -917,15 +921,18 @@ class QAPISchemaEnumType(QAPISchemaType):
>          return c_name(self.name)
>
>      def c_null(self):
> -        return c_enum_const(self.name, (self.values + ['_MAX'])[0],
> -                            self.prefix)
> +        if self.values:
> +            value = self.values[0].name
> +        else:
> +            value = '_MAX'
> +        return c_enum_const(self.name, value, self.prefix)

Works.  The alternative would be casting 0 to the enumeration type.

>
>      def json_type(self):
>          return 'string'
>
>      def visit(self, visitor):
>          visitor.visit_enum_type(self.name, self.info,
> -                                self.values, self.prefix)
> +                                [v.name for v in self.values], self.prefix)

This keeps the change local.

The alternative is changing visit_enum_type().  Precedence:
visit_object_type() takes an array of QAPISchemaObjectTypeMember.  Could
also be done later if we find we want the consistency after all.

>
>
>  class QAPISchemaArrayType(QAPISchemaType):
> @@ -1049,6 +1056,8 @@ class QAPISchemaMember(object):
>              else:
>                  assert owner.endswith('-wrapper')
>                  return '(branch of %s)' % owner[:-8]
> +        if owner.endswith('Kind'):
> +            return '(branch of %s)' % owner[:-4]
>          return '(%s of %s)' % (self.role, owner)

Let's add a comment pointing to _make_implicit_enum_type(), similar to
the existing one above that points to _make_implicit_object_type().

>
>      def describe(self):
> @@ -1094,12 +1103,13 @@ class QAPISchemaObjectTypeVariants(object):
>              self.tag_member = seen[c_name(self.tag_name)]
>              assert self.tag_name == self.tag_member.name
>          assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> +        tag_values = [v.name for v in self.tag_member.type.values]
>          for v in self.variants:
>              v.check(schema)
>              # Union names must match enum values; alternate names are
>              # checked separately. Use 'seen' to tell the two apart.
>              if seen:
> -                assert v.name in self.tag_member.type.values
> +                assert v.name in tag_values

Could search self.tag_member.type.values directly instead, but I can't
find an way to express it that can compete with your code in
readability.

>                  assert isinstance(v.type, QAPISchemaObjectType)
>                  v.type.check(schema)
>
> @@ -1257,15 +1267,16 @@ class QAPISchema(object):
>          self.the_empty_object_type = QAPISchemaObjectType(':empty', None, 
> None,
>                                                            [], None)
>          self._def_entity(self.the_empty_object_type)
> -        self._def_entity(QAPISchemaEnumType('QType', None,
> -                                            ['none', 'qnull', 'qint',
> -                                             'qstring', 'qdict', 'qlist',
> -                                             'qfloat', 'qbool'],
> +        qtype_values = [QAPISchemaMember(name) for name in
> +                        ['none', 'qnull', 'qint', 'qstring', 'qdict', 
> 'qlist',
> +                         'qfloat', 'qbool']]

Let's wrap after 'qdict'.

> +        self._def_entity(QAPISchemaEnumType('QType', None, qtype_values,
>                                              'QTYPE'))
>
>      def _make_implicit_enum_type(self, name, info, values):
>          name = name + 'Kind'   # Use namespace reserved by add_name()
> -        self._def_entity(QAPISchemaEnumType(name, info, values, None))
> +        self._def_entity(QAPISchemaEnumType(
> +            name, info, [QAPISchemaMember(v) for v in values], None))
>          return name
>
>      def _make_array_type(self, element_type, info):
> @@ -1288,7 +1299,8 @@ class QAPISchema(object):
>          name = expr['enum']
>          data = expr['data']
>          prefix = expr.get('prefix')
> -        self._def_entity(QAPISchemaEnumType(name, info, data, prefix))
> +        self._def_entity(QAPISchemaEnumType(
> +            name, info, [QAPISchemaMember(v) for v in data], prefix))

Third instance of the list comprehension mapping a list of names to a
list of QAPISchemaMember.  Would a helper function make sense?

>
>      def _make_member(self, name, typ, info):
>          optional = False

I'm generally reluctant to add classes, but this one seems to pan out
nicely.