Eric Blake <ebl...@redhat.com> 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.
