Eric Blake <ebl...@redhat.com> writes:
> 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.
Another concern: we should take care not to generate reserved
identifiers.
* Potential issue with your proposal: identifiers that begin with an
underscore and either an uppercase letter or another underscore are
always reserved for any use.
* Existing issue: downstream extensions carry a __RFQDN_ prefix in the
schema, which map to reserved C identifiers.
Example: qapi-schema-test.json type '__org.qemu_x-Enum' generates
typedef enum __org_qemu_x_Enum {
ORG_QEMU_X_ENUM___ORG_QEMU_X_VALUE = 0,
ORG_QEMU_X_ENUM_MAX = 1,
} __org_qemu_x_Enum;
extern const char *const __org_qemu_x_Enum_lookup[];
>>> 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.
I can't see a need now, but if one arises, we could do it.
>> * 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.
Good :)
>>>> + 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
I only have time for a quick glance now. It looks lovely!
