Re: [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union

[Markus Armbruster]

Wenchao Xia <address@hidden> writes:

> 于 2014/2/21 0:38, Markus Armbruster 写道:
>> Wenchao Xia <address@hidden> writes:
>> 
>>> By default, any union will automatically generate a enum type as
>>> "[UnionName]Kind" in C code, and it is duplicated when the discriminator
>>> is specified as a pre-defined enum type in schema. After this patch,
>>> the pre-defined enum type will be really used as the switch case
>>> condition in generated C code, if discriminator is an enum field.
>>>
>>> Signed-off-by: Wenchao Xia <address@hidden>
>>> ---
>>>   docs/qapi-code-gen.txt |    8 ++++++--
>>>   scripts/qapi-types.py  |   20 ++++++++++++++++----
>>>   scripts/qapi-visit.py  |   27 ++++++++++++++++++++-------
>>>   scripts/qapi.py        |   13 ++++++++++++-
>>>   4 files changed, 54 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>>> index 0728f36..a2e7921 100644
>>> --- a/docs/qapi-code-gen.txt
>>> +++ b/docs/qapi-code-gen.txt
>>> @@ -123,11 +123,15 @@ And it looks like this on the wire:
>>>   
>>>   Flat union types avoid the nesting on the wire. They are used whenever a
>>>   specific field of the base type is declared as the discriminator ('type' 
>>> is
>>> -then no longer generated). The discriminator must always be a string field.
>>> +then no longer generated). The discriminator can be a string field or a
>>> +predefined enum field. If it is a string field, a hidden enum type will be
>>> +generated as "[UNION_NAME]Kind". If it is an enum field, a compile time 
>>> check
>>> +will be done to verify the correctness. It is recommended to use an enum 
>>> field.
>>>   The above example can then be modified as follows:
>>>   
>>> + { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
>>>    { 'type': 'BlockdevCommonOptions',
>>> -   'data': { 'driver': 'str', 'readonly': 'bool' } }
>>> +   'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
>>>    { 'union': 'BlockdevOptions',
>>>      'base': 'BlockdevCommonOptions',
>>>      'discriminator': 'driver',
>>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>>> index 656a9a0..4098c60 100644
>>> --- a/scripts/qapi-types.py
>>> +++ b/scripts/qapi-types.py
>>> @@ -201,14 +201,22 @@ def generate_union(expr):
>>>       base = expr.get('base')
>>>       discriminator = expr.get('discriminator')
>>>   
>>> +    expr_elem = {'expr': expr}
>>> +    enum_define = discriminator_find_enum_define(expr_elem)
>> 
>> expr_elem has no fp, line.  What if discriminator_find_enum_define
>> throws a QAPIExprError?
>> 
>   It shouldn't happen, since all error check happens in parse_schema().

Impossible errors often mean the abstractions aren't quite right.  But
this series is progress, and I don't want to delay it by demanding
perfection.  We can improve on it in tree if we want.

>> More of the same below.
[...]
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index 130dced..2a5eb59 100644
>>> --- a/scripts/qapi.py
>>> +++ b/scripts/qapi.py
>>> @@ -250,11 +250,22 @@ def parse_schema(fp):
>>>               add_enum(expr['enum'], expr['data'])
>>>           elif expr.has_key('union'):
>>>               add_union(expr)
>>> -            add_enum('%sKind' % expr['union'])
>>>           elif expr.has_key('type'):
>>>               add_struct(expr)
>>>           exprs.append(expr)
>>>   
>>> +    # Try again for hidden UnionKind enum
>>> +    for expr_elem in schema.exprs:
>>> +        expr = expr_elem['expr']
>>> +        if expr.has_key('union'):
>>> +            try:
>>> +                enum_define = discriminator_find_enum_define(expr_elem)
>>> +            except QAPIExprError, e:
>>> +                print >>sys.stderr, e
>>> +                exit(1)
>>> +            if not enum_define:
>>> +                add_enum('%sKind' % expr['union'])
>>> +
>>>       try:
>>>           check_exprs(schema)
>>>       except QAPIExprError, e:
>> 
>> I guess you move this into its own loop because when base types are used
>> before they're defined, or an enum type is used for a discriminator
>> before it's defined, then discriminator_find_enum_define() complains.
>> Correct?
>> 
>   Exactly, which allow enum define after usage in schema.

Do we want to (have to?) support "use before define" in schemas?  Eric,
what do you think?

If yes, we should add suitable tests.  Outside the scope of this series.