Message ID | 1446052473-19170-7-git-send-email-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Eric Blake <eblake@redhat.com> writes: > These two methods are now close enough that we can finally merge > them, relying on the fact that simple unions now provide a > reasonable local_members. Change gen_struct() to gen_object() > that handles all forms of QAPISchemaObjectType, and rename and > shrink gen_union() to gen_variants() to handle the portion of > gen_object() needed when variants are present. > > gen_struct_fields() now has a single caller, so it no longer > needs an optional parameter; however, I did not choose to inline > it into the caller. > > No difference to generated code. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v8: new patch > --- > scripts/qapi-types.py | 36 +++++++++++------------------------- > 1 file changed, 11 insertions(+), 25 deletions(-) > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index a6bf95d..403768b 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -51,7 +51,7 @@ def gen_struct_field(member): > return ret > > > -def gen_struct_fields(local_members, base=None): > +def gen_struct_fields(local_members, base): > ret = '' > > if base: > @@ -70,7 +70,7 @@ def gen_struct_fields(local_members, base=None): > return ret > > > -def gen_struct(name, base, members): > +def gen_object(name, base, members, variants): > ret = mcgen(''' > > struct %(c_name)s { > @@ -79,11 +79,14 @@ struct %(c_name)s { > > ret += gen_struct_fields(members, base) > > + if variants: > + ret += gen_variants(variants) > + > # Make sure that all structs have at least one field; this avoids > # potential issues with attempting to malloc space for zero-length > # structs in C, and also incompatibility with C++ (where an empty > # struct is size 1). > - if not (base and base.members) and not members: > + if not (base and base.members) and not members and not variants: > ret += mcgen(''' > char qapi_dummy_field_for_empty_struct; > ''') > @@ -140,17 +143,7 @@ const int %(c_name)s_qtypes[QTYPE_MAX] = { > return ret > > > -def gen_union(name, base, variants): > - ret = mcgen(''' > - > -struct %(c_name)s { > -''', > - c_name=c_name(name)) > - if base: > - ret += gen_struct_fields([], base) > - else: > - ret += gen_struct_field(variants.tag_member) > - > +def gen_variants(variants): > # FIXME: What purpose does data serve, besides preventing a union that > # has a branch named 'data'? We use it in qapi-visit.py to decide > # whether to bypass the switch statement if visiting the discriminator > @@ -159,11 +152,11 @@ struct %(c_name)s { > # should not be any data leaks even without a data pointer. Or, if > # 'data' is merely added to guarantee we don't have an empty union, > # shouldn't we enforce that at .json parse time? > - ret += mcgen(''' > + ret = mcgen(''' > union { /* union tag is @%(c_name)s */ > void *data; > ''', > - c_name=c_name(variants.tag_member.name)) > + c_name=c_name(variants.tag_member.name)) > > for var in variants.variants: > # Ugly special case for simple union TODO get rid of it > @@ -176,7 +169,6 @@ struct %(c_name)s { > > ret += mcgen(''' > } u; > -}; > ''') > > return ret > @@ -268,13 +260,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): > > def visit_object_type(self, name, info, base, members, variants): > self._fwdecl += gen_fwd_object_or_array(name) > - if variants: > - 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) > + self.decl += gen_object(name, base, members, variants) > if base: > self.decl += gen_upcast(name, base) > self._gen_type_cleanup(name) > @@ -282,7 +268,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): > def visit_alternate_type(self, name, info, variants): > self._fwdecl += gen_fwd_object_or_array(name) > self._fwdefn += gen_alternate_qtypes(name, variants) > - self.decl += gen_union(name, None, variants) > + self.decl += gen_object(name, None, [variants.tag_member], variants) > self.decl += gen_alternate_qtypes_decl(name) > self._gen_type_cleanup(name) Turned out nicely. We could morph gen_struct_field() back into its original shape in a separate patch: def gen_struct_fields(members): ret = '' for memb in members: ret += gen_struct_field(memb.name, memb.type, memb.optional) return ret with calling code ret += mcgen(''' /* Members inherited from %(c_name)s: */ ''', c_name=base.c_name()) ret += gen_struct_fields(base.members) ret += mcgen(''' /* Own members: */ ''') ret += gen_struct_fields(local_members) Matter of taste.
On 10/30/2015 07:01 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> These two methods are now close enough that we can finally merge >> them, relying on the fact that simple unions now provide a >> reasonable local_members. Change gen_struct() to gen_object() >> that handles all forms of QAPISchemaObjectType, and rename and >> shrink gen_union() to gen_variants() to handle the portion of >> gen_object() needed when variants are present. >> >> gen_struct_fields() now has a single caller, so it no longer >> needs an optional parameter; however, I did not choose to inline >> it into the caller. >> >> No difference to generated code. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> def visit_object_type(self, name, info, base, members, variants): >> self._fwdecl += gen_fwd_object_or_array(name) >> - if variants: >> - 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) >> + self.decl += gen_object(name, base, members, variants) >> if base: >> self.decl += gen_upcast(name, base) >> self._gen_type_cleanup(name) >> @@ -282,7 +268,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): >> def visit_alternate_type(self, name, info, variants): >> self._fwdecl += gen_fwd_object_or_array(name) >> self._fwdefn += gen_alternate_qtypes(name, variants) >> - self.decl += gen_union(name, None, variants) >> + self.decl += gen_object(name, None, [variants.tag_member], variants) >> self.decl += gen_alternate_qtypes_decl(name) >> self._gen_type_cleanup(name) > > Turned out nicely. Yes, I was pretty pleased with it. It gives more credence to our choice of introspection representation. > > We could morph gen_struct_field() back into its original shape in a > separate patch: > > def gen_struct_fields(members): > ret = '' > > for memb in members: > ret += gen_struct_field(memb.name, memb.type, memb.optional) > return ret > > with calling code > > ret += mcgen(''' > /* Members inherited from %(c_name)s: */ > ''', > c_name=base.c_name()) > ret += gen_struct_fields(base.members) > ret += mcgen(''' > /* Own members: */ > ''') > ret += gen_struct_fields(local_members) > > Matter of taste. Reasonable idea; I'll add it in for the v9 spin.
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index a6bf95d..403768b 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -51,7 +51,7 @@ def gen_struct_field(member): return ret -def gen_struct_fields(local_members, base=None): +def gen_struct_fields(local_members, base): ret = '' if base: @@ -70,7 +70,7 @@ def gen_struct_fields(local_members, base=None): return ret -def gen_struct(name, base, members): +def gen_object(name, base, members, variants): ret = mcgen(''' struct %(c_name)s { @@ -79,11 +79,14 @@ struct %(c_name)s { ret += gen_struct_fields(members, base) + if variants: + ret += gen_variants(variants) + # Make sure that all structs have at least one field; this avoids # potential issues with attempting to malloc space for zero-length # structs in C, and also incompatibility with C++ (where an empty # struct is size 1). - if not (base and base.members) and not members: + if not (base and base.members) and not members and not variants: ret += mcgen(''' char qapi_dummy_field_for_empty_struct; ''') @@ -140,17 +143,7 @@ const int %(c_name)s_qtypes[QTYPE_MAX] = { return ret -def gen_union(name, base, variants): - ret = mcgen(''' - -struct %(c_name)s { -''', - c_name=c_name(name)) - if base: - ret += gen_struct_fields([], base) - else: - ret += gen_struct_field(variants.tag_member) - +def gen_variants(variants): # FIXME: What purpose does data serve, besides preventing a union that # has a branch named 'data'? We use it in qapi-visit.py to decide # whether to bypass the switch statement if visiting the discriminator @@ -159,11 +152,11 @@ struct %(c_name)s { # should not be any data leaks even without a data pointer. Or, if # 'data' is merely added to guarantee we don't have an empty union, # shouldn't we enforce that at .json parse time? - ret += mcgen(''' + ret = mcgen(''' union { /* union tag is @%(c_name)s */ void *data; ''', - c_name=c_name(variants.tag_member.name)) + c_name=c_name(variants.tag_member.name)) for var in variants.variants: # Ugly special case for simple union TODO get rid of it @@ -176,7 +169,6 @@ struct %(c_name)s { ret += mcgen(''' } u; -}; ''') return ret @@ -268,13 +260,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): def visit_object_type(self, name, info, base, members, variants): self._fwdecl += gen_fwd_object_or_array(name) - if variants: - 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) + self.decl += gen_object(name, base, members, variants) if base: self.decl += gen_upcast(name, base) self._gen_type_cleanup(name) @@ -282,7 +268,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): def visit_alternate_type(self, name, info, variants): self._fwdecl += gen_fwd_object_or_array(name) self._fwdefn += gen_alternate_qtypes(name, variants) - self.decl += gen_union(name, None, variants) + self.decl += gen_object(name, None, [variants.tag_member], variants) self.decl += gen_alternate_qtypes_decl(name) self._gen_type_cleanup(name)
These two methods are now close enough that we can finally merge them, relying on the fact that simple unions now provide a reasonable local_members. Change gen_struct() to gen_object() that handles all forms of QAPISchemaObjectType, and rename and shrink gen_union() to gen_variants() to handle the portion of gen_object() needed when variants are present. gen_struct_fields() now has a single caller, so it no longer needs an optional parameter; however, I did not choose to inline it into the caller. No difference to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> --- v8: new patch --- scripts/qapi-types.py | 36 +++++++++++------------------------- 1 file changed, 11 insertions(+), 25 deletions(-)