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
signature.asc
Description: OpenPGP digital signature
