Re: [Qemu-devel] [PATCH v9 07/17] qapi: Start converting to new qapi union layout

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 10/22/2015 07:54 AM, Markus Armbruster wrote:
>
>> 
>> This is clever and ugly in equal measure.  I respect that.  Fortunately,
>> it's also temporary.
>> 
>>> Flat unions do not need the anonymous union for the tag member,
>>> as we already fixed that to use the member name instead of 'kind'
>>> back in commit 0f61af3e.
>> 
>> Unlike then, we need multiple commits for simple unions, because they're
>> more widely used?
>
> Yes. In fact, both you and I expressed surprise back then that the main
> body of qemu didn't need adjusting - our only use of flat unions was
> hidden behind QDict manipulations rather than direct generated qapi
> struct, explaining why nothing was affected when we converted flat
> unions.  But a useful note for the commit message at any rate.
>
>> 
>>>                           On the other hand, the duplication
>>> means that we temporarily cannot support 'u' as a branch name.
>> 
>> Separate paragraph, because now you're talking about the *other*
>> anonymous union.
>> 
>>> Later, when the conversions are complete, we will remove the
>>> duplication hacks and restore support for 'u' as a branch name.
>
> And based on comments on 3/17, I'm deferring any testsuite changes
> related to 'u' collisions until after this conversion to inline base is
> complete, so this part of the commit message actually disappears in v10
> because I'm no longer touching qapi-schema-test this early.
>
>>>
>>> Note, however, that we do not rename the generated enum, which
>>> is still 'FooKind'.  A further patch could generate implicit
>>> enums as 'FooType', but that causes more churn to C code, and
>>> gets harder since the generator already reserved the '*Kind'
>>> namespace, but there are already QMP constructs with '*Type'
>>> naming which means we cannot easily reserve it for qapi.
>> 
>> Oh, we can reserve whatever we want in QAPI, it's just a lot of churn to
>> adapt the QAPI-using code.
>> 
>> I'd simply say "but that would cause substantial churn to C code, as
>> there are already QAPI definitions with '*Type' naming".
>
> Okay.
>
>
>>>      for var in variants.variants:
>>>          # Ugly special case for simple union TODO get rid of it
>>>          typ = var.simple_union_type() or var.type
>>> -        ret += mcgen('''
>>> +        tmp += mcgen('''
>>>          %(c_type)s %(c_name)s;
>>>  ''',
>>>                       c_type=typ.c_type(),
>>>                       c_name=c_name(var.name))
>>>
>>> +    ret += tmp
>>> +    ret += '    ' + '\n    '.join(tmp.split('\n'))
>>>      ret += mcgen('''
>>> +    } u;
>>>      };
>>>  };
>>>  ''')
>> 
>> It took me some head-scratching to understand why this generates
>> correctly indented output.  If it wasn't temporary code, I'd ask for
>> cleanup.
>
> Would a comment help?  It's because we add 4 spaces after each newline,
> but need an indent prior to the first line of tmp, and the '} u;' line
> picks up four spaces after the last line of tmp.

Yes.  Let's not worry about it, it's just temporary scaffolding.

>> Second part: convert qapi-visit.py.  Not mentioned in commit message.
>> Separate patch, perhaps?
>
> Sure, I could split.
>
>
>> 
>> Third part: work around temporary clash with 'u'.  Needs to remain in
>> this patch, obviously.  Suggest to amend the commit message to say
>> 
>>     On the other hand, the duplication means that we temporarily cannot
>>     support 'u' as a branch name.  Adapt a few tests that do.
>
> Or, rather, dropped entirely, because the tests for collisions with 'u'
> will be deferred until after the conversion is complete.

If deferring is easy, go for it.