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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3612 bytes --]

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.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]