Re: [Qemu-devel] [PATCH RFC v2 27/47] qapi-visit: Convert to QAPISchemaVisitor, fixing bugs

[Markus Armbruster]

Eric Blake <address@hidden> 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!