| Message ID | 1446052473-19170-6-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > We were previously creating all unions with an empty list for > local_members. However, it will make it easier to unify struct > and union generation if we include the generated tag member in > local_members. That way, we can have a common code pattern: > visit the base (if any), visit the local members (if any), visit > the variants (if any). The local_members of a flat union > remains empty (because the discriminator is already visited as > part of the base). Keeping the implicit tag implicit by not including it in local_members was a conscious design decision, but if including it makes unifying struct and union into objects easier, go right ahead. > The various front end entities then map as follows: > struct: optional base, optional local_members, no variants > simple union: no base, one-element local_members, variants with tag_member > from local_members > flat union: base, no local_members, variants with tag_member from base > alternate: no base, no local_members, variants > > With the new local members, we require a bit of finesse to > avoid assertions in the clients. No change to generated code. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v8: new patch > --- > scripts/qapi-types.py | 4 +++- > scripts/qapi-visit.py | 4 +++- > scripts/qapi.py | 14 ++++++++++---- > tests/qapi-schema/qapi-schema-test.out | 2 ++ > tests/qapi-schema/union-clash-data.out | 1 + > tests/qapi-schema/union-empty.out | 1 + > 6 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index b37900f..a6bf95d 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -269,7 +269,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): > def visit_object_type(self, name, info, base, members, variants): > self._fwdecl += gen_fwd_object_or_array(name) > if variants: > - assert not members # not implemented > + if members: > + assert len(members) == 1 > + assert members[0] == variants.tag_member > self.decl += gen_union(name, base, variants) > else: > self.decl += gen_struct(name, base, members) The assertion checks that not passing members to gen_union() won't lose any. Before the patch, we assert there are none. After the patch, we assert there's either none or variants.tag_member. Before ans after, gen_union() takes care of variants.tag_member. Okay. Let's keep the comment, though. Perhaps like this: # Members other than variants.tag_member not implemented assert len(members) == 1 assert members[0] == variants.tag_member I hope this the whole conditional will eventually be replaced by self.decl += gen_object(name, base, members, variants) > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index f40c3c7..318b8e6 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -360,7 +360,9 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): > def visit_object_type(self, name, info, base, members, variants): > self.decl += gen_visit_decl(name) > if variants: > - assert not members # not implemented > + if members: > + assert len(members) == 1 > + assert members[0] == variants.tag_member > self.defn += gen_visit_union(name, base, variants) > else: > self.defn += gen_visit_struct(name, base, members) Likewise, let's keep the comment. > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 7c50cc4..84ac151 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -957,6 +957,9 @@ class QAPISchemaArrayType(QAPISchemaType): > > class QAPISchemaObjectType(QAPISchemaType): > def __init__(self, name, info, base, local_members, variants): > + # struct has local_members, optional base, and no variants > + # flat union has base, variants, and no local_members > + # simple union has local_members, variants, and no base > QAPISchemaType.__init__(self, name, info) > assert base is None or isinstance(base, str) > for m in local_members: > @@ -1048,9 +1051,11 @@ class QAPISchemaObjectTypeVariants(object): > self.variants = variants > > def check(self, schema, members, seen): > - if self.tag_name: > + if self.tag_name: # flat union > self.tag_member = seen[self.tag_name] > - else: > + elif seen: # simple union > + assert self.tag_member in seen.itervalues() > + else: # alternate > self.tag_member.check(schema, members, seen) > assert isinstance(self.tag_member.type, QAPISchemaEnumType) > for v in self.variants: The test for simple union is hackish. I guess you want to bypass self.tag_member.check() when for simple unions, because it's now checked when QAPISchemaObjectType.check() calls QAPISchemaObjectTypeMember.check() for each of self.local_members. Could we instead make QAPISchemaAlternateType.check() call QAPISchemaObjectTypeMember.check() for its implicit tag, and drop the else here? > @@ -1270,13 +1275,14 @@ class QAPISchema(object): > if tag_name: > variants = [self._make_variant(key, value) > for (key, value) in data.iteritems()] > + members = [] > else: > variants = [self._make_simple_variant(key, value, info) > for (key, value) in data.iteritems()] > tag_member = self._make_implicit_tag(name, info, variants) > + members = [tag_member] > self._def_entity( > - QAPISchemaObjectType(name, info, base, > - self._make_members(OrderedDict(), info), > + QAPISchemaObjectType(name, info, base, members, > QAPISchemaObjectTypeVariants(tag_name, > tag_member, > variants))) Two steps squashed together: 1. Simplify _make_members(OrderedDict(), info) to []. 2. Put a simple union's implicit tag_member into the members array passed to QAPISchemaObjectType(). Okay. [...]
On 10/30/2015 06:54 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> We were previously creating all unions with an empty list for >> local_members. However, it will make it easier to unify struct >> and union generation if we include the generated tag member in >> local_members. That way, we can have a common code pattern: >> visit the base (if any), visit the local members (if any), visit >> the variants (if any). The local_members of a flat union >> remains empty (because the discriminator is already visited as >> part of the base). > > Keeping the implicit tag implicit by not including it in local_members > was a conscious design decision, but if including it makes unifying > struct and union into objects easier, go right ahead. > >> The various front end entities then map as follows: >> struct: optional base, optional local_members, no variants >> simple union: no base, one-element local_members, variants with tag_member >> from local_members >> flat union: base, no local_members, variants with tag_member from base >> alternate: no base, no local_members, variants >> >> With the new local members, we require a bit of finesse to >> avoid assertions in the clients. No change to generated code. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> +++ b/scripts/qapi-types.py >> @@ -269,7 +269,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): >> def visit_object_type(self, name, info, base, members, variants): >> self._fwdecl += gen_fwd_object_or_array(name) >> if variants: >> - assert not members # not implemented >> + if members: >> + assert len(members) == 1 >> + assert members[0] == variants.tag_member >> self.decl += gen_union(name, base, variants) >> else: >> self.decl += gen_struct(name, base, members) > > The assertion checks that not passing members to gen_union() won't lose > any. Before the patch, we assert there are none. After the patch, we > assert there's either none or variants.tag_member. Before ans after, > gen_union() takes care of variants.tag_member. Okay. > > Let's keep the comment, though. Perhaps like this: > > # Members other than variants.tag_member not implemented > assert len(members) == 1 > assert members[0] == variants.tag_member Indeed, that sounds nicer. > > I hope this the whole conditional will eventually be replaced by > > self.decl += gen_object(name, base, members, variants) Yes. It goes away in 6/17 for qapi-types (as you already saw), and I have a later patch queued that likewise makes it go away for qapi-visit (but not in subset C or D, since there's still a bit more to clean up there first). >> def check(self, schema, members, seen): >> - if self.tag_name: >> + if self.tag_name: # flat union >> self.tag_member = seen[self.tag_name] >> - else: >> + elif seen: # simple union >> + assert self.tag_member in seen.itervalues() >> + else: # alternate >> self.tag_member.check(schema, members, seen) >> assert isinstance(self.tag_member.type, QAPISchemaEnumType) >> for v in self.variants: > > The test for simple union is hackish. > > I guess you want to bypass self.tag_member.check() when for simple > unions, because it's now checked when QAPISchemaObjectType.check() calls > QAPISchemaObjectTypeMember.check() for each of self.local_members. > > Could we instead make QAPISchemaAlternateType.check() call > QAPISchemaObjectTypeMember.check() for its implicit tag, and drop the > else here? I debated about that when writing my patch. Yes, I can make that change (it seems a little bit unclean to be calling self.variants.tag_member.check() in QAPISchemaAlternateType, but not too much worse; and adding a comment might help).
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index b37900f..a6bf95d 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -269,7 +269,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): def visit_object_type(self, name, info, base, members, variants): self._fwdecl += gen_fwd_object_or_array(name) if variants: - assert not members # not implemented + if members: + assert len(members) == 1 + assert members[0] == variants.tag_member self.decl += gen_union(name, base, variants) else: self.decl += gen_struct(name, base, members) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index f40c3c7..318b8e6 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -360,7 +360,9 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): def visit_object_type(self, name, info, base, members, variants): self.decl += gen_visit_decl(name) if variants: - assert not members # not implemented + if members: + assert len(members) == 1 + assert members[0] == variants.tag_member self.defn += gen_visit_union(name, base, variants) else: self.defn += gen_visit_struct(name, base, members) diff --git a/scripts/qapi.py b/scripts/qapi.py index 7c50cc4..84ac151 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -957,6 +957,9 @@ class QAPISchemaArrayType(QAPISchemaType): class QAPISchemaObjectType(QAPISchemaType): def __init__(self, name, info, base, local_members, variants): + # struct has local_members, optional base, and no variants + # flat union has base, variants, and no local_members + # simple union has local_members, variants, and no base QAPISchemaType.__init__(self, name, info) assert base is None or isinstance(base, str) for m in local_members: @@ -1048,9 +1051,11 @@ class QAPISchemaObjectTypeVariants(object): self.variants = variants def check(self, schema, members, seen): - if self.tag_name: + if self.tag_name: # flat union self.tag_member = seen[self.tag_name] - else: + elif seen: # simple union + assert self.tag_member in seen.itervalues() + else: # alternate self.tag_member.check(schema, members, seen) assert isinstance(self.tag_member.type, QAPISchemaEnumType) for v in self.variants: @@ -1270,13 +1275,14 @@ class QAPISchema(object): if tag_name: variants = [self._make_variant(key, value) for (key, value) in data.iteritems()] + members = [] else: variants = [self._make_simple_variant(key, value, info) for (key, value) in data.iteritems()] tag_member = self._make_implicit_tag(name, info, variants) + members = [tag_member] self._def_entity( - QAPISchemaObjectType(name, info, base, - self._make_members(OrderedDict(), info), + QAPISchemaObjectType(name, info, base, members, QAPISchemaObjectTypeVariants(tag_name, tag_member, variants))) diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index e20a823..786024e 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -132,6 +132,7 @@ object UserDefFlatUnion2 case value2: UserDefB case value3: UserDefA object UserDefNativeListUnion + member type: UserDefNativeListUnionKind optional=False case integer: :obj-intList-wrapper case s8: :obj-int8List-wrapper case s16: :obj-int16List-wrapper @@ -187,6 +188,7 @@ object __org.qemu_x-Struct object __org.qemu_x-Struct2 member array: __org.qemu_x-Union1List optional=False object __org.qemu_x-Union1 + member type: __org.qemu_x-Union1Kind optional=False case __org.qemu_x-branch: :obj-str-wrapper enum __org.qemu_x-Union1Kind ['__org.qemu_x-branch'] object __org.qemu_x-Union2 diff --git a/tests/qapi-schema/union-clash-data.out b/tests/qapi-schema/union-clash-data.out index 6277239..cea8551 100644 --- a/tests/qapi-schema/union-clash-data.out +++ b/tests/qapi-schema/union-clash-data.out @@ -2,5 +2,6 @@ object :empty object :obj-int-wrapper member data: int optional=False object TestUnion + member type: TestUnionKind optional=False case data: :obj-int-wrapper enum TestUnionKind ['data'] diff --git a/tests/qapi-schema/union-empty.out b/tests/qapi-schema/union-empty.out index 8b5a7bf..9c89fd1 100644 --- a/tests/qapi-schema/union-empty.out +++ b/tests/qapi-schema/union-empty.out @@ -1,3 +1,4 @@ object :empty object Union + member type: UnionKind optional=False enum UnionKind []
We were previously creating all unions with an empty list for local_members. However, it will make it easier to unify struct and union generation if we include the generated tag member in local_members. That way, we can have a common code pattern: visit the base (if any), visit the local members (if any), visit the variants (if any). The local_members of a flat union remains empty (because the discriminator is already visited as part of the base). The various front end entities then map as follows: struct: optional base, optional local_members, no variants simple union: no base, one-element local_members, variants with tag_member from local_members flat union: base, no local_members, variants with tag_member from base alternate: no base, no local_members, variants With the new local members, we require a bit of finesse to avoid assertions in the clients. No change to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> --- v8: new patch --- scripts/qapi-types.py | 4 +++- scripts/qapi-visit.py | 4 +++- scripts/qapi.py | 14 ++++++++++---- tests/qapi-schema/qapi-schema-test.out | 2 ++ tests/qapi-schema/union-clash-data.out | 1 + tests/qapi-schema/union-empty.out | 1 + 6 files changed, 20 insertions(+), 6 deletions(-)