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

From: Eric Blake <eblake@redhat.com>
To: Max Reitz <mreitz@redhat.com>, qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org, Markus Armbruster <armbru@redhat.com>,
	Kevin Wolf <kwolf@redhat.com>,
	Michael Roth <mdroth@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH 01/13] qapi: Add default-variant for flat unions
Date: Fri, 11 May 2018 13:13:28 -0500	[thread overview]
Message-ID: <a89fba24-836e-8c1a-f50c-8564b486b9f0@redhat.com> (raw)
In-Reply-To: <4c7382dc-724f-4420-ea8d-b4d1bf0edf84@redhat.com>

On 05/11/2018 12:59 PM, Max Reitz wrote:
> 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?

Umm, yeah ;)

In fact, I think it is as simple as:

if variants:
     if variants.default_tag_value:
         ret += mcgen('''
if (!obj->has_%(c_name)s) {
     obj->has_%(c_name)s = true;
     obj->%(c_name)s = %(enum_const)s;
}
''')
     ret += mcgen('''
switch (obj->%(c_name)s) {
...

and you are back to not needing a temporary variable.

> 
>> 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.

My preference - special-casing "is missing" is prone to abuse.  I don't 
want to support that if we can at all avoid it.  The sane semantics is 
that the default is populated as soon as we detect that something is 
missing, and whether the user relies on the default by leaving the 
discriminator absent, or explicitly supplies the discriminator set to 
the default, the behavior should always be the same.

> 
> And that's the tricky part: I think it's hard to say that a missing
> discriminator never has special meaning.

It won't, if we decide right now that we don't want to let it :)

>  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.

And that's the way I'm leaning.

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

Markus, what's your preference?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org