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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2285 bytes --]

On 02/17/2016 05:05 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> When we added support for a user-specified prefix for an enum
>> type (commit 351d36e), we forgot to teach the qapi-visit code
>> to honor that prefix in the case of using a prefixed enum as
>> the discriminator for a flat union.  While there is still some
>> on-list debate on whether we want to keep prefixes, we should
>> at least make it work as long as it is still part of the code
>> base.
>>

> 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)

Yes, that would be nicer, but more invasive...

> 
> * 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.

...and that's why the quick patch now rather than a more invasive
refactor is reasonable.  I've added this refactor to my list of things
that might be worth doing some later day, but certainly not before the
current queue is flushed.

> 
> * 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.
> 
> Bottom line for this patch: the fix is complete.

Good, your analysis matched mine.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]