[Qemu-devel] [PATCH v11 13/28] qapi: Hoist tag collision check to Variants.check()
Checking that a given QAPISchemaObjectTypeVariant.name is a member of the corresponding QAPISchemaEnumType of the owning QAPISchemaObjectTypeVariants.tag_member ensures that there are no collisions in the generated C union for those tag values (since the enum itself should have no collisions). However, this check was the only thing that Variant.check() was doing beyond the work of the superclass ObjectTypeMember.check(), and resulted in a difference of the .check() signatures just to pass the enum type down. Simplify things by instead doing the tag name check as part of Variants.check(), at which point we can rely on inheritance instead of overriding Variant.check(). Signed-off-by: Eric Blake --- v11: don't use tag_type local variable, rebase to v.type.check() v10: new patch --- scripts/qapi.py | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 296b9bb..c6f3fce 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1059,7 +1059,8 @@ class QAPISchemaObjectTypeVariants(object): self.tag_member = seen[self.tag_name] assert isinstance(self.tag_member.type, QAPISchemaEnumType) for v in self.variants: -v.check(schema, self.tag_member.type) +v.check(schema) +assert v.name in self.tag_member.type.values if isinstance(v.type, QAPISchemaObjectType): v.type.check(schema) @@ -1075,10 +1076,6 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): def __init__(self, name, typ): QAPISchemaObjectTypeMember.__init__(self, name, typ, False) -def check(self, schema, tag_type): -QAPISchemaObjectTypeMember.check(self, schema) -assert self.name in tag_type.values - # This function exists to support ugly simple union special cases # TODO get rid of them, and drop the function def simple_union_type(self): -- 2.4.3
Re: [Qemu-devel] [PATCH v11 13/28] qapi: Hoist tag collision check to Variants.check()
Eric Blake writes: > Checking that a given QAPISchemaObjectTypeVariant.name is a > member of the corresponding QAPISchemaEnumType of the owning > QAPISchemaObjectTypeVariants.tag_member ensures that there are > no collisions in the generated C union for those tag values > (since the enum itself should have no collisions). > > However, this check was the only thing that Variant.check() was > doing beyond the work of the superclass ObjectTypeMember.check(), Since PATCH 05, actually. Suggest to mention that. > and resulted in a difference of the .check() signatures just to > pass the enum type down. > > Simplify things by instead doing the tag name check as part of > Variants.check(), at which point we can rely on inheritance > instead of overriding Variant.check(). > > Signed-off-by: Eric Blake > > --- > v11: don't use tag_type local variable, rebase to v.type.check() > v10: new patch > --- > scripts/qapi.py | 7 ++- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 296b9bb..c6f3fce 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -1059,7 +1059,8 @@ class QAPISchemaObjectTypeVariants(object): > self.tag_member = seen[self.tag_name] > assert isinstance(self.tag_member.type, QAPISchemaEnumType) > for v in self.variants: > -v.check(schema, self.tag_member.type) > +v.check(schema) > +assert v.name in self.tag_member.type.values > if isinstance(v.type, QAPISchemaObjectType): > v.type.check(schema) > > @@ -1075,10 +1076,6 @@ class > QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): > def __init__(self, name, typ): > QAPISchemaObjectTypeMember.__init__(self, name, typ, False) > > -def check(self, schema, tag_type): > -QAPISchemaObjectTypeMember.check(self, schema) > -assert self.name in tag_type.values > - > # This function exists to support ugly simple union special cases > # TODO get rid of them, and drop the function > def simple_union_type(self): QAPISchemaObjectTypeVariant is now an almost trivial variation of QAPISchemaObjectTypeMember. Differences: * __init__() has no parameter optional * Method simple_union_type(), which exists only to support pointless differences in code generation for simple unions, all marked TODO. There's hope we can get rid of the class after the TODOs are resolved.
Re: [Qemu-devel] [PATCH v11 13/28] qapi: Hoist tag collision check to Variants.check()
On 11/11/2015 06:56 AM, Markus Armbruster wrote:
> Eric Blake writes:
>
>> Checking that a given QAPISchemaObjectTypeVariant.name is a
>> member of the corresponding QAPISchemaEnumType of the owning
>> QAPISchemaObjectTypeVariants.tag_member ensures that there are
>> no collisions in the generated C union for those tag values
>> (since the enum itself should have no collisions).
>>
>> However, this check was the only thing that Variant.check() was
>> doing beyond the work of the superclass ObjectTypeMember.check(),
>
> Since PATCH 05, actually. Suggest to mention that.
I debated about munging patch 5 or 6 to actually make this change there;
but decided that separate is just fine. But yes, mentioning the earlier
commit title will help.
>> @@ -1075,10 +1076,6 @@ class
>> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>> def __init__(self, name, typ):
>> QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
>>
>> -def check(self, schema, tag_type):
>> -QAPISchemaObjectTypeMember.check(self, schema)
>> -assert self.name in tag_type.values
>> -
>> # This function exists to support ugly simple union special cases
>> # TODO get rid of them, and drop the function
>> def simple_union_type(self):
>
> QAPISchemaObjectTypeVariant is now an almost trivial variation of
> QAPISchemaObjectTypeMember. Differences:
>
> * __init__() has no parameter optional
>
> * Method simple_union_type(), which exists only to support pointless
> differences in code generation for simple unions, all marked TODO.
>
> There's hope we can get rid of the class after the TODOs are resolved.
Nope. Because in 15/28 I add back in a non-trivial difference of "role =
'branch'" compared to the superclass "role = 'member'", which affects
the quality of error messages using .describe().
That, and I still have no idea how to get rid of the TODO (we know how
to convert simple unions to flat unions, but the conversion is an
either-or choice: either we keep the C layout the same but change {} in
the JSON representation, or we keep QMP the same but affect the C layout
- so fixing the TODO has to resolve that discrepancy, and may end up
touching lots of code).
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v11 13/28] qapi: Hoist tag collision check to Variants.check()
Eric Blake writes:
> On 11/11/2015 06:56 AM, Markus Armbruster wrote:
>> Eric Blake writes:
>>
>>> Checking that a given QAPISchemaObjectTypeVariant.name is a
>>> member of the corresponding QAPISchemaEnumType of the owning
>>> QAPISchemaObjectTypeVariants.tag_member ensures that there are
>>> no collisions in the generated C union for those tag values
>>> (since the enum itself should have no collisions).
>>>
>>> However, this check was the only thing that Variant.check() was
>>> doing beyond the work of the superclass ObjectTypeMember.check(),
>>
>> Since PATCH 05, actually. Suggest to mention that.
>
> I debated about munging patch 5 or 6 to actually make this change there;
> but decided that separate is just fine. But yes, mentioning the earlier
> commit title will help.
>
>>> @@ -1075,10 +1076,6 @@ class
>>> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>>> def __init__(self, name, typ):
>>> QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
>>>
>>> -def check(self, schema, tag_type):
>>> -QAPISchemaObjectTypeMember.check(self, schema)
>>> -assert self.name in tag_type.values
>>> -
>>> # This function exists to support ugly simple union special cases
>>> # TODO get rid of them, and drop the function
>>> def simple_union_type(self):
>>
>> QAPISchemaObjectTypeVariant is now an almost trivial variation of
>> QAPISchemaObjectTypeMember. Differences:
>>
>> * __init__() has no parameter optional
>>
>> * Method simple_union_type(), which exists only to support pointless
>> differences in code generation for simple unions, all marked TODO.
>>
>> There's hope we can get rid of the class after the TODOs are resolved.
>
> Nope. Because in 15/28 I add back in a non-trivial difference of "role =
> 'branch'" compared to the superclass "role = 'member'", which affects
> the quality of error messages using .describe().
>
> That, and I still have no idea how to get rid of the TODO (we know how
> to convert simple unions to flat unions, but the conversion is an
> either-or choice: either we keep the C layout the same but change {} in
> the JSON representation, or we keep QMP the same but affect the C layout
> - so fixing the TODO has to resolve that discrepancy, and may end up
> touching lots of code).
I'm not afraid of touching lots of QEMU code when I have a good reason.
Anyway, it's not something we should worry about right now.
