Re: [Qemu-block] [PATCH 01/13] qapi: Add default-variant for flat unions

[Max Reitz]

On 2018-05-10 15:12, Eric Blake wrote:
> On 05/09/2018 11:55 AM, Max Reitz wrote:
>> This patch allows specifying a discriminator that is an optional member
>> of the base struct.  In such a case, a default value must be provided
>> that is used when no value is given.
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>   qapi/introspect.json           |  8 ++++++
>>   scripts/qapi/common.py         | 57
>> ++++++++++++++++++++++++++++++++++--------
>>   scripts/qapi/doc.py            |  8 ++++--
>>   scripts/qapi/introspect.py     | 10 +++++---
>>   scripts/qapi/visit.py          | 33 ++++++++++++++++++++++--
>>   tests/qapi-schema/test-qapi.py |  2 ++
>>   6 files changed, 101 insertions(+), 17 deletions(-)
> 
> I've been threatening that we might need this for some time, so I'm glad
> to see it being implemented.  We'll see if the tests in 2 and 3 cover
> the code added here.
> 
>>
>> diff --git a/qapi/introspect.json b/qapi/introspect.json
>> index c7f67b7d78..2d7b1e3745 100644
>> --- a/qapi/introspect.json
>> +++ b/qapi/introspect.json
>> @@ -168,6 +168,13 @@
>>   # @tag: the name of the member serving as type tag.
>>   #       An element of @members with this name must exist.
>>   #
>> +# @default-variant: if the @tag element of @members is optional, this
>> +#                   is the default value for choosing a variant.  Its
>> +#                   value must be a valid value for @tag.
> 
> Perhaps s/must/will/ as this struct is used for output (and therefore we
> always meet the condition, rather than the user having to do something
> correctly).

I mostly copied from the other descriptions which seemed to prefer
"must", but I'm happy with either.

> Nice that you remembered introspection.

I didn't, because I had no idea how introspection works exactly before
this series.  But one of the tests broke, thus telling me I might have
forgotten something. :-)

>> +#                   Present exactly when @tag is present and the
>> +#                   associated element of @members is optional.
>> +#                   (Since: 2.13)
>> +#
>>   # @variants: variant members, i.e. additional members that
>>   #            depend on the type tag's value.  Present exactly when
>>   #            @tag is present.  The variants are in no particular order,

[...]

>>   diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>> index 5d72d8936c..ecffc46bd3 100644
>> --- a/scripts/qapi/visit.py
>> +++ b/scripts/qapi/visit.py
>> @@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members,
>> variants):
>>   void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj,
>> Error **errp)
>>   {
>>       Error *err = NULL;
>> -
>>   ''',
>>                   c_name=c_name(name))
>>   +    if variants:
>> +        ret += mcgen('''
>> +    %(c_type)s %(c_name)s;
>> +''',
>> +                     c_type=variants.tag_member.type.c_name(),
>> +                     c_name=c_name(variants.tag_member.name))
>> +
>> +    ret += mcgen('''
>> +
>> +''')
>> +
> 
> Creating a temp variable makes it easier to handle the default...
> 
>>       if base:
>>           ret += mcgen('''
>>       visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, &err);
>> @@ -75,8 +85,27 @@ void visit_type_%(c_name)s_members(Visitor *v,
>> %(c_name)s *obj, Error **errp)
>>   ''')
>>         if variants:
>> +        if variants.default_tag_value is None:
>> +            ret += mcgen('''
>> +    %(c_name)s = obj->%(c_name)s;
>> +''',
>> +                         c_name=c_name(variants.tag_member.name))
>> +        else:
>> +            ret += mcgen('''
>> +    if (obj->has_%(c_name)s) {
>> +        %(c_name)s = obj->%(c_name)s;
>> +    } else {
>> +        %(c_name)s = %(enum_const)s;
>> +    }
>> +''',
>> +                         c_name=c_name(variants.tag_member.name),
>> +                         enum_const=c_enum_const(
>> +                             variants.tag_member.type.name,
>> +                             variants.default_tag_value,
>> +                             variants.tag_member.type.prefix))
>> +
>>           ret += mcgen('''
>> -    switch (obj->%(c_name)s) {
>> +    switch (%(c_name)s) {
> 
> ...compared to the old code that just inlined the one thing that used to
> be assigned to what is now the temporary variable.
> 
> It might be possible to inline things so that the generated code reads
> either:
> 
> switch (obj->discriminator) {
> switch (!obj->has_discriminator ? DEFAULT : obj->discriminator) {
> 
> but I don't think it's worth the effort; and the temporary variable is
> fine even though it makes the generated file bigger.

I don't really mind either way, but depending on the default value and
the discriminator name, using ?: may lead to a rather long line.

>>   ''',
>>                        c_name=c_name(variants.tag_member.name))
>>   diff --git a/tests/qapi-schema/test-qapi.py
>> b/tests/qapi-schema/test-qapi.py
>> index c1a144ba29..f2a072b92e 100644
>> --- a/tests/qapi-schema/test-qapi.py
>> +++ b/tests/qapi-schema/test-qapi.py
>> @@ -56,6 +56,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>>       def _print_variants(variants):
>>           if variants:
>>               print('    tag %s' % variants.tag_member.name)
>> +            if variants.default_tag_value:
>> +                print('    default variant: %s' %
>> variants.default_tag_value)
>>               for v in variants.variants:
>>                   print('    case %s: %s' % (v.name, v.type.name))
>>  
> 
> Looks good!
> Reviewed-by: Eric Blake <address@hidden>

Phew. :-)

Thanks!

Max

signature.asc
Description: OpenPGP digital signature