Re: [Qemu-devel] [PATCH] qapi-visit: Honor prefix of discriminator enum

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/17/2016 05:05 AM, Markus Armbruster wrote:
>> Let's check for completeness.  Calls of c_enum_const():
>> 
>> * QAPISchemaEnumType.c_null() and (with your patch) gen_visit_union()
>>   call it like
>> 
>>     c_enum_const(TYPE.name, MEMBER, TYPE.prefix)
>> 
>>   where MEMBER is a member of enumeration type TYPE.
>> 
>>   As your patch shows, the prefix is easy to forget.  A safer function
>>   would take just TYPE and MEMBER:
>> 
>>     TYPE.c_member(MEMBER)
>> 
>
> I took a look at this email again today, and while it still sounds like
> a nice action, I wasn't able to get it done quickly (certainly not 2.6
> material, at any rate).
>
>> * gen_event_send() calls
>> 
>>     c_enum_const(event_enum_name, name)
>> 
>>   where name is member of the enum type named event_enum_name.  That's
>>   okay because the type is auto-generated without a prefix.  Regardless,
>>   we could do something like
>> 
>>     schema.lookup_type(event_enum_name).c_member(name)
>> 
>>   Requires actually constructing the type, which is probably a good idea
>>   anyway, because it gets us the necessary collision checks.  Replacing
>>   global event_enum_name by event_enum_type would be nice then.  Out of
>>   scope for this patch.
>> 
>> * gen_enum_lookup() and gen_enum() work on name, values, prefix instead
>>   of the type.  I figure they do to support qapi-event.py.  If we clean
>>   it up to create the type, these functions could use the type as well,
>>   and then c_enum_const() could be dropped.
>
> Well, they also do it to support qapi-types.py, where the
> visit_enum_type() visitor callback has already broken things out into
> name/values/prefix instead of providing a type, and where we don't have
> easy access to the schema to do schema.lookup_type(name).
>
> As I've worked more with the visitors, I'm really wondering if
> visit_enum_type(self, type, info) would be a saner callback interface
> than visit_enum_type(self, type.name, info, type.values, type.prefix)

Maybe it is, but I'm not sure.  The existing visit_enum_type() forces
discipline: you can't just use whatever property of the type you like.
Discipline is good until you overdo it and it becomes a straightjacket.

Let's not worry about it now, but concentrate on keeping the existing,
lengthy QAPI queue moving.