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 <[email protected]> >> --- >> 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 <[email protected]>
Phew. :-)
Thanks!
Max
signature.asc
Description: OpenPGP digital signature
