[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 14/15] qapi: Allow anonymous branch types in
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v7 14/15] qapi: Allow anonymous branch types in flat union |
Date: |
Thu, 16 Jun 2016 16:33:57 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> Recent commits added support for an anonymous type as the base
> of a flat union; with a bit more work, we can also allow an
> anonymous struct as a branch of a flat union. This probably
> most useful when a branch adds no additional members beyond the
> common elements of the base (that is, the branch struct is '{}'),
> but can be used for any struct in the same way we allow for an
> anonymous struct for a command.
>
> The generator has to do a bit of special-casing for the fact that
> we do not emit a '_empty' struct nor a 'visit_type__empty_members()'
> corresponding to the special ':empty' type; but when the branch
> is truly empty, there's nothing to do.
Well, it could emit them, if it makes things easier.
> The testsuite gets an update to use the new feature, and to ensure
> that we can still detect invalid collisions of QMP names.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v7: new patch
> ---
> scripts/qapi.py | 21 +++++++++++++++------
> scripts/qapi-types.py | 10 +++++++---
> scripts/qapi-visit.py | 14 ++++++++++----
> tests/Makefile | 1 +
> tests/qapi-schema/flat-union-inline.err | 2 +-
> tests/qapi-schema/flat-union-inline.json | 5 ++---
> tests/qapi-schema/qapi-schema-test.json | 5 +++--
> tests/qapi-schema/qapi-schema-test.out | 8 ++++++--
> tests/qapi-schema/union-inline.err | 1 +
> tests/qapi-schema/union-inline.exit | 1 +
> tests/qapi-schema/union-inline.json | 4 ++++
> tests/qapi-schema/union-inline.out | 0
> 12 files changed, 51 insertions(+), 21 deletions(-)
> create mode 100644 tests/qapi-schema/union-inline.err
> create mode 100644 tests/qapi-schema/union-inline.exit
> create mode 100644 tests/qapi-schema/union-inline.json
> create mode 100644 tests/qapi-schema/union-inline.out
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 7d568d9..4c531e7 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -607,9 +607,11 @@ def check_union(expr, expr_info):
> for (key, value) in members.items():
> check_name(expr_info, "Member of union '%s'" % name, key)
>
> - # Each value must name a known type
> + # Each value must name a type; although the type may be anonymous
> + # for a flat union.
> check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
> - value, allow_array=not base, allow_metas=allow_metas)
> + value, allow_array=not base, allow_dict=base is not None,
> + allow_optional=True, allow_metas=allow_metas)
>
> # If the discriminator names an enum type, then all members
> # of 'data' must also be members of the enum type.
> @@ -1061,6 +1063,9 @@ class QAPISchemaMember(object):
> return '(parameter of %s)' % owner[:-4]
> elif owner.endswith('-base'):
> return '(base of %s)' % owner[:-5]
> + elif owner.endswith('-branch'):
> + return ('(member of %s branch %s)'
> + % tuple(owner[:-7].split(':')))
I think we should point to the spot that puts in the colon, and back.
Do we really need the "of %s" part?
> else:
> assert owner.endswith('-wrapper')
> # Unreachable and not implemented
> @@ -1335,7 +1340,11 @@ class QAPISchema(object):
> self._make_members(data, info),
> None))
>
> - def _make_variant(self, case, typ):
> + def _make_variant(self, case, typ, info, owner):
> + if isinstance(typ, dict):
> + typ = self._make_implicit_object_type(
> + "%s:%s" % (owner, case), info, 'branch',
> + self._make_members(typ, info)) or 'q_empty'
This is the spot.
Is this the best spot for creating the type? Precedent:
_make_simple_variant() creates a wrapper type. Because of that, I'm
inclined to answer yes. But let's examine the caller. They're visible
right below.
> return QAPISchemaObjectTypeVariant(case, typ)
>
> def _make_simple_variant(self, case, typ, info):
> @@ -1356,7 +1365,7 @@ class QAPISchema(object):
> base = (self._make_implicit_object_type(
> name, info, 'base', self._make_members(base, info)))
> if tag_name:
> - variants = [self._make_variant(key, value)
> + variants = [self._make_variant(key, value, info, name)
> for (key, value) in data.iteritems()]
> members = []
> else:
This is the caller we need to extend to cope with an anonymous type.
The anonymous type arrives in form of @value being a dict rather than a
type name. Possible, because the change to check_union() now permits
dict in addition to string.
> @@ -1375,7 +1384,7 @@ class QAPISchema(object):
> def _def_alternate_type(self, expr, info):
> name = expr['alternate']
> data = expr['data']
> - variants = [self._make_variant(key, value)
> + variants = [self._make_variant(key, value, info, name)
> for (key, value) in data.iteritems()]
> tag_member = QAPISchemaObjectTypeMember('type', 'QType', False)
> self._def_entity(
Here, dict still can't happen, but passing more argument's isn't exactly
a burden.
> @@ -1485,7 +1494,7 @@ def c_enum_const(type_name, const_name, prefix=None):
> type_name = prefix
> return camel_to_upper(type_name) + '_' + c_name(const_name,
> False).upper()
>
> -c_name_trans = string.maketrans('.-', '__')
> +c_name_trans = string.maketrans('.-:', '___')
Because you use the colon as separator. Hmm.
>
>
> # Map @name to a valid C identifier.
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 5a9e2da..f1edab2 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -61,7 +61,8 @@ def gen_object(name, base, members, variants):
def gen_object(name, base, members, variants):
if name in objects_seen:
return ''
objects_seen.add(name)
> ret = ''
> if variants:
> for v in variants.variants:
> - if isinstance(v.type, QAPISchemaObjectType):
> + if (isinstance(v.type, QAPISchemaObjectType)
> + and not (v.type.is_implicit() and v.type.is_empty())):
> ret += gen_object(v.type.name, v.type.base,
> v.type.local_members, v.type.variants)
>
This is the recursion that ensures an object type's variant member types
are emitted before the object type.
We can't simply .type == schema.the_empty_object_type like
qapi-introspect.py does, because we don't have schema handy here. Hmm.
Do we really need this change? Note that gen_object() does nothing for
name in objects_seen, and we do this in visit_begin():
# gen_object() is recursive, ensure it doesn't visit the empty type
objects_seen.add(schema.the_empty_object_type.name)
> @@ -123,11 +124,14 @@ def gen_variants(variants):
> c_name=c_name(variants.tag_member.name))
>
> for var in variants.variants:
Here, we emit the C union member for a variant:
> + typ = var.type.c_unboxed_type()
> + if (isinstance(var.type, QAPISchemaObjectType) and
> + var.type.is_empty() and var.type.is_implicit()):
> + typ = 'char'
> ret += mcgen('''
> %(c_type)s %(c_name)s;
> ''',
> - c_type=var.type.c_unboxed_type(),
> - c_name=c_name(var.name))
> + c_type=typ, c_name=c_name(var.name))
Your change replaces the C type when var.type is the empty type.
Without that, we'd get 'q_empty', I guess.
Do we need the union member? Hmm, we may want to avoid empty unions, to
avoid pedantic warnings.
Should we make the_empty_object_type.c_unboxed_type() return a suitable
C type instead of 'q_empty'?
>
> ret += mcgen('''
> } u;
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 07ae6d1..46f8b39 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -79,13 +79,19 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s
> *obj, Error **errp)
> for var in variants.variants:
> ret += mcgen('''
> case %(case)s:
Here, we emit the visit of the "active" C union member:
> - visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
> - break;
> ''',
> case=c_enum_const(variants.tag_member.type.name,
> var.name,
> - variants.tag_member.type.prefix),
> - c_type=var.type.c_name(), c_name=c_name(var.name))
> + variants.tag_member.type.prefix))
> + if (not isinstance(var.type, QAPISchemaObjectType) or
> + not var.type.is_empty()):
> + ret += mcgen('''
> + visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
> +''',
> + c_type=var.type.c_name(),
> c_name=c_name(var.name))
> + ret += mcgen('''
> + break;
> +''')
Your change suppresses the visit_type_FOO_members() call when var.type
is the empty type. Without that, we'd get visit_type_q_empty_members(),
which doesn't exist.
Why not make it exist?
>
> ret += mcgen('''
> default:
> diff --git a/tests/Makefile b/tests/Makefile
> index 5cd6177..d7d9597 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -379,6 +379,7 @@ qapi-schema += union-base-no-discriminator.json
> qapi-schema += union-branch-case.json
> qapi-schema += union-clash-branches.json
> qapi-schema += union-empty.json
> +qapi-schema += union-inline.json
> qapi-schema += union-invalid-base.json
> qapi-schema += union-optional-branch.json
> qapi-schema += union-unknown.json
> diff --git a/tests/qapi-schema/flat-union-inline.err
> b/tests/qapi-schema/flat-union-inline.err
> index 2333358..efcafec 100644
> --- a/tests/qapi-schema/flat-union-inline.err
> +++ b/tests/qapi-schema/flat-union-inline.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-inline.json:7: Member 'value1' of union
> 'TestUnion' should be a type name
> +tests/qapi-schema/flat-union-inline.json:6: 'kind' (member of TestUnion
> branch value2) collides with 'kind' (member of Base)
> diff --git a/tests/qapi-schema/flat-union-inline.json
> b/tests/qapi-schema/flat-union-inline.json
> index 62c7cda..a049ec8 100644
> --- a/tests/qapi-schema/flat-union-inline.json
> +++ b/tests/qapi-schema/flat-union-inline.json
> @@ -1,5 +1,4 @@
> -# we require branches to be a struct name
> -# TODO: should we allow anonymous inline branch types?
> +# we allow anonymous union branches, but they must not have clashing names
> { 'enum': 'TestEnum',
> 'data': [ 'value1', 'value2' ] }
> { 'struct': 'Base',
> @@ -8,4 +7,4 @@
> 'base': 'Base',
> 'discriminator': 'enum1',
> 'data': { 'value1': { 'string': 'str' },
> - 'value2': { 'integer': 'int' } } }
> + 'value2': { 'kind': 'int' } } }
> diff --git a/tests/qapi-schema/qapi-schema-test.json
> b/tests/qapi-schema/qapi-schema-test.json
> index df91f3d..5128b49 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -23,7 +23,7 @@
> # for testing override of default naming heuristic
> { 'enum': 'QEnumTwo',
> 'prefix': 'QENUM_TWO',
> - 'data': [ 'value1', 'value2' ] }
> + 'data': [ 'value1', 'value2', 'value3' ] }
>
> # for testing nested structs
> { 'struct': 'UserDefOne',
> @@ -81,7 +81,8 @@
> 'base': { '*integer': 'int', 'string': 'str', 'enum1': 'QEnumTwo' },
> 'discriminator': 'enum1',
> 'data': { 'value1' : 'UserDefC', # intentional forward reference
> - 'value2' : 'UserDefB' } }
You're deleting the equally intentional case of a backward reference :)
> + 'value2' : { },
> + 'value3' : { 'boolean': 'bool', '*number': 'number' } } }
>
> { 'struct': 'WrapAlternate',
> 'data': { 'alt': 'UserDefAlternate' } }
> diff --git a/tests/qapi-schema/qapi-schema-test.out
> b/tests/qapi-schema/qapi-schema-test.out
> index 8a00c6b..7fac2da 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -50,7 +50,7 @@ object NestedEnumsOne
> member enum2: EnumOne optional=True
> member enum3: EnumOne optional=False
> member enum4: EnumOne optional=True
> -enum QEnumTwo ['value1', 'value2']
> +enum QEnumTwo ['value1', 'value2', 'value3']
> prefix QENUM_TWO
> enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat',
> 'qbool']
> prefix QTYPE
> @@ -82,7 +82,8 @@ object UserDefFlatUnion2
> base q_obj_UserDefFlatUnion2-base
> tag enum1
> case value1: UserDefC
> - case value2: UserDefB
> + case value2: q_empty
> + case value3: q_obj_UserDefFlatUnion2:value3-branch
> object UserDefNativeListUnion
> member type: UserDefNativeListUnionKind optional=False
> tag type
> @@ -179,6 +180,9 @@ object q_obj_UserDefFlatUnion2-base
> member integer: int optional=True
> member string: str optional=False
> member enum1: QEnumTwo optional=False
> +object q_obj_UserDefFlatUnion2:value3-branch
> + member boolean: bool optional=False
> + member number: number optional=True
> object q_obj___org.qemu_x-command-arg
> member a: __org.qemu_x-EnumList optional=False
> member b: __org.qemu_x-StructList optional=False
> diff --git a/tests/qapi-schema/union-inline.err
> b/tests/qapi-schema/union-inline.err
> new file mode 100644
> index 0000000..6c5389a
> --- /dev/null
> +++ b/tests/qapi-schema/union-inline.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/union-inline.json:2: Member 'value1' of union 'TestUnion'
> should be a type name
> diff --git a/tests/qapi-schema/union-inline.exit
> b/tests/qapi-schema/union-inline.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/union-inline.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/union-inline.json
> b/tests/qapi-schema/union-inline.json
> new file mode 100644
> index 0000000..b8c5df6
> --- /dev/null
> +++ b/tests/qapi-schema/union-inline.json
> @@ -0,0 +1,4 @@
> +# simple unions cannot have anonymous branches (only flat unions can)
> +{ 'union': 'TestUnion',
> + 'data': { 'value1': { 'string': 'str' },
> + 'value2': { 'kind': 'int' } } }
> diff --git a/tests/qapi-schema/union-inline.out
> b/tests/qapi-schema/union-inline.out
> new file mode 100644
> index 0000000..e69de29
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v7 14/15] qapi: Allow anonymous branch types in flat union,
Markus Armbruster <=