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

[Max Reitz]

On 2018-05-10 15:18, Eric Blake wrote:
> On 05/10/2018 08:12 AM, Eric Blake wrote:
> 
> Oh, I just had a thought:
> 
>>> +++ b/scripts/qapi/visit.py
>>> @@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members, 
> 
>>>       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;
> 
> In this branch of code, is it worth also generating:
> 
> %has_(c_name)s = true;

You mean obj->has_%(c_name)s, and then also set obj->%(c_name)s?

> That way, the rest of the C code does not have to check
> has_discriminator, because the process of assigning the default will
> ensure that has_discriminator is always true later on.  It does have the
> effect that output would never omit the discriminator - but that might
> be a good thing: if we ever have an output union that used to have a
> mandatory discriminator and want to now make it optional, we don't want
> to break older clients that expected the discriminator to be present. It
> does obscure whether input relied on the default, but I don't think that
> hurts.

Also, it would make code a bit simpler because it can cover the !has_X
case under X == default_X.

But OTOH, you could make that case for any optional value -- except
where "is missing" has special value.

And that's the tricky part: I think it's hard to say that a missing
discriminator never has special meaning.  We only need the
default-variant so we know which variant of the union to pick; but we
don't know that the fact that the discriminator value was missing does
not have special meaning.

Of course, we can simply foreclose that by setting it here.

I don't have money in this game, so I suppose it's yours and Markus's
call. :-)

Max

signature.asc
Description: OpenPGP digital signature