[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH RFC v2 27/47] qapi-visit: Convert to QAPISchemaV

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH RFC v2 27/47] qapi-visit: Convert to QAPISchemaVisitor, fixing bugs
Date: Mon, 27 Jul 2015 13:01:22 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0

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:

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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]