Re: [Qemu-devel] Problem with discriminated unions with enum prefixes

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/16/2016 10:35 AM, Daniel P. Berrange wrote:
>> In my LUKS encryption series, I have a discriminated union for
>> storing options for different encryption formats. See qapi/crypto.json
>> in this file:
>> 
>>   https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg03187.html
>> 
>> You'll notice I have the 'prefix' line for the enum commented out. When
>> I uncomment this, I discovered that the discriminated union visitor does
>> not deal with prefixes.
>> 
>
>> Apply that and then try to build and it'll fail with:
>> 
>> qapi-visit.c: In function ‘visit_type_QDemo’:
>> qapi-visit.c:7596:10: error: ‘Q_DEMO_TYPE_FOO’ undeclared (first use in this 
>> function)
>>      case Q_DEMO_TYPE_FOO:
>>           ^
>> qapi-visit.c:7596:10: note: each undeclared identifier is reported only once 
>> for each function it appears in
>> qapi-visit.c:7599:10: error: ‘Q_DEMO_TYPE_BAR’ undeclared (first use in this 
>> function)
>>      case Q_DEMO_TYPE_BAR:
>>           ^
>> 
>> The issue is that we used the 'QDEMO_TYPE' custom prefix for generating the
>> enum, but we didn't use the prefix in the union visitor.
>
> Should be a quick fix, if we want to keep prefixes.  I'll go ahead and
> post it, at least for discussion purposes.
>
>> 
>> I know we had had previous discussions with Markus strongly wanting to kill
>> off the support for enum prefixes. So before I waste time trying to fix
>> this union visitor code to handle prefixes, I figure we should decide if
>> we actually want to fix it, or go with Markus' plan to kill custom prefixes
>> on enums.
>
> I'm still on the fence which way to go; we've definitely improved the
> code base so that inadvertent collisions due to odd heuristics are less
> likely to occur, but every special case we have to carry (custom prefix
> being one of them) results in more code to maintain and test.

Yes.

As so many things, QAPI started out relatively simple & stupid, if
saddled with a certain amount of accidental complexity due to hasty
design and implementation.  Then features got bolted on left and right.
That's normal; useful software expands to fill available space.

We've been working hard on reducing the accidental complexity.  I
believe we should also try to control the complexity due to features.
Features need to earn their keep.

>> Per previous discussions, I think the ability to have custom prefixes is
>> quite desirable, to get more natural enum constant names. At the end of
>> the day though, the default enum naming is far from the worst bit of
>> QEMU, so I'm not ultimately too bothered either way. We either make
>> custom enum prefixes work everything they need to, or remove them.

Keeping an unloved feature half-broken while we deliberate whether to
keep it is the worst of all worlds.  Since the fix is a one-liner (plus
tests), let's just apply it to unblock Dan as soon as possible.