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 <mre...@redhat.com> >> --- >> 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 <ebl...@redhat.com> Phew. :-) Thanks! Max
signature.asc
Description: OpenPGP digital signature