On 07/27/2015 11:53 AM, Markus Armbruster wrote:

>> Oh, and that means our generator has a collision bug that none of my
>> added tests have exposed yet: you cannot have a base class and
>> simultaneously add a member named 'base':
>>
>> { 'struct': 'Base', 'data': { 'i': 'int' } }
>> { 'struct': 'Sub', 'base': 'Base', 'data': { 'base': 'str' } }
>>
>> because the generated C code is trying to use the name 'base' for its
>> own purposes.
> 
> *sigh*
> 
>>                I guess that means more pre-req patches to the series to
>> expose the bug, and either tighten the parser to reject things for now
>> (easiest) or update the generator to not collide (harder, and fine for a
>> later series).
> 
> Yes.
> 
> Life would be easier if the original authors had adopted sane naming
> conventions from the start.

Like reserving ourselves a namespace based on single _ for internal use.
 We practically already have that - all user names either start with a
letter or double underscore, so we could use single (and triple)
underscore for internally-generated purposes, freeing up 'base',
'*Kind', '*_MAX', and other namespace abuses back to the user.  Well, we
may also need to reserve mid-name double-underscore (that is, the user
can only supply double underscore at the beginning, but not middle, of
an identifier).  Ah well, food for thought for later patches.


>> Okay, I see where you are using .flat from the initial parse.  I still
>> think it is a bit odd that you are defining '.flat' for each 'variant'
>> within 'variants', even though, for a given 'variants', all members will
>> have the same setting of '.flat'.  That makes me wonder if '.flat'
>> should belong instead to the top-level 'variants' struct rather than to
>> each 'variant' member.
> 
> Two reasons for putting flat where it is:
> 
> * The philosophical one: from the generator's point of view, there's no
>   fundamental reason why all variants need to be flat or none.  The
>   generator really doesn't care.

And we may decide to exploit that down the road to allow some sort of
qapi syntax for explicitly designating a union branch as flat or boxed,
rather than the current approach of the type of union determining the
status of all branch members.

> 
> * The pragmatic one (a.k.a. the real one): there are places where I use
>   v.flat, but don't have the variants owning v handy.
> 
>> But again I wonder what would happen if you had instead normalized the
>> input of simple unions into always having an implicit struct (with
>> single member 'data'), so that by the time you get here, you only have
>> to deal with a single representation of unions instead of having to
>> still emit different things for flat vs. simple (since on the wire, we
>> already proved simple is shorthand that can be duplicated by a flat union).
> 
> I hope we can get there!  But at this stage of the conversion, I want to
> minimize output change, and .flat makes preserving all its warts much
> easier.

Agreed.  By the end of the series, I was convinced that the use of
.flat, at least in this series, makes sense.


>>> +    disc_key = variants.tag_member.name
>>> +    if not variants.tag_name:
>>> +        # we pointlessly use a different key for simple unions
>>
>> We could fix that (as a separate patch); wonder how much C code it would
>> affect.  A lot of these things that we can alter in generated code are
>> certainly easier to see now that we have a clean generator :)
> 
> Yup, the warts stand out now.

And I've already demonstrated what sort of cleanups can be done to
attack some of the warts:
https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05266.html

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