Eric Blake <ebl...@redhat.com> writes:
> On 11/11/2015 06:56 AM, Markus Armbruster wrote:
>> Eric Blake <ebl...@redhat.com> 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.
