Re: [PATCH v3 2/3] qapi: Do not generate empty enum

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>> > Per the C++ standard, empty enum are ill-formed. Do not generate

The C standard.  The C++ standard doesn't apply here :)

>> > them in order to avoid:
>> >
>> >   In file included from qga/qga-qapi-emit-events.c:14:
>> >   qga/qga-qapi-emit-events.h:20:1: error: empty enum is invalid
>> >      20 | } qga_QAPIEvent;
>> >         | ^
>> >
>> > Reported-by: Markus Armbruster <armbru@redhat.com>
>> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> 
>> Two failures in "make check-qapi-schema" (which is run by "make check"):
>> 
>> 1. Positive test case qapi-schema-test
>> 
>>     --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/qapi-schema-test.out
>>     +++ 
>>     @@ -19,7 +19,6 @@
>>          member enum2: EnumOne optional=True
>>          member enum3: EnumOne optional=False
>>          member enum4: EnumOne optional=True
>>     -enum MyEnum
>>      object Empty1
>>      object Empty2
>>          base Empty1
>> 
>>    You forgot to update expected test output.  No big deal.
>> 
>> 2. Negative test case union-empty
>> 
>>     --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/union-empty.err
>>     +++ 
>>     @@ -1,2 +1,2 @@
>>     -union-empty.json: In union 'Union':
>>     -union-empty.json:4: union has no branches
>>     +union-empty.json: In struct 'Base':
>>     +union-empty.json:3: member 'type' uses unknown type 'Empty'
>>     stderr:
>>     qapi-schema-test FAIL
>>     union-empty FAIL
>> 
>>    The error message regresses.
>> 
>>    I can see two ways to fix this:
>> 
>>    (A) You can't just drop empty enumeration types on the floor.  To not
>>        generate code for them, you need to skip them wherever we
>>        generate code for enumeration types.
>> 
>>    (B) Outlaw empty enumeration types.
>> 
>> I recommend to give (B) a try, it's likely simpler.
>
> Possible trap-door with (B), if we have any enums where *every*
> member is conditionalized on a CONFIG_XXX rule, there might be
> certain build scenarios where an enum suddenly becomes empty.

True.  Scratch the idea.

Trap-door also applies to (A): we can still end up with empty enums.

(C) Always emit a dummy member.  This is actually what we do now:

    typedef enum OnOffAuto {
        ON_OFF_AUTO_AUTO = 1,
        ON_OFF_AUTO_ON = 2,
        ON_OFF_AUTO_OFF = 3,
        ON_OFF_AUTO__MAX,               <--- the dummy
    } OnOffAuto;

But the next patch changes it to

    typedef enum OnOffAuto {
        ON_OFF_AUTO_AUTO,
        ON_OFF_AUTO_ON,
        ON_OFF_AUTO_OFF,
    #define ON_OFF_AUTO__MAX 3
    } OnOffAuto;

Two problems, actually.

One, we lose the dummy.  We could add one back like

    typedef enum OnOffAuto {
        ON_OFF_AUTO__DUMMY = 0,
        ON_OFF_AUTO_AUTO = 0,
        ON_OFF_AUTO_ON,
        ON_OFF_AUTO_OFF,
    #define ON_OFF_AUTO__MAX 3
    } OnOffAuto;

But all of this falls apart with conditional members!

Example 1 (taken from qapi/block-core.json):

    { 'enum': 'BlockdevAioOptions',
      'data': [ 'threads', 'native',
                { 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' } ] }

Generates now:

    typedef enum BlockdevAioOptions {
        BLOCKDEV_AIO_OPTIONS_THREADS,
        BLOCKDEV_AIO_OPTIONS_NATIVE,
    #if defined(CONFIG_LINUX_IO_URING)
        BLOCKDEV_AIO_OPTIONS_IO_URING,
    #endif /* defined(CONFIG_LINUX_IO_URING) */
        BLOCKDEV_AIO_OPTIONS__MAX,
    } BlockdevAioOptions;

BLOCKDEV_AIO_OPTIONS__MAX is 3 if defined(CONFIG_LINUX_IO_URING), else
2.

After the next patch:

    typedef enum BlockdevAioOptions {
        BLOCKDEV_AIO_OPTIONS_THREADS,
        BLOCKDEV_AIO_OPTIONS_NATIVE,
    #if defined(CONFIG_LINUX_IO_URING)
        BLOCKDEV_AIO_OPTIONS_IO_URING,
    #endif /* defined(CONFIG_LINUX_IO_URING) */
    #define BLOCKDEV_AIO_OPTIONS__MAX 3
    } BlockdevAioOptions;

Now it's always 3.

Example 2 (same with members reordered):

    { 'enum': 'BlockdevAioOptions',
      'data': [ { 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' },
                'threads', 'native' ] }

Same problem for __MAX, additional problem for __DUMMY:

    typedef enum BlockdevAioOptions {
        BLOCKDEV_AIO_OPTIONS__DUMMY = 0,
    #if defined(CONFIG_LINUX_IO_URING)
        BLOCKDEV_AIO_OPTIONS_IO_URING = 0,
    #endif /* defined(CONFIG_LINUX_IO_URING) */
        BLOCKDEV_AIO_OPTIONS_THREADS,
        BLOCKDEV_AIO_OPTIONS_NATIVE,
    #define BLOCKDEV_AIO_OPTIONS__MAX 3
    } BlockdevAioOptions;

If CONFIG_LINUX_IO_URING is off, the enum starts at 1 instead of 0.

Arrays indexed by the enum start with a hole.  Code using them is
probably not prepared for holes.

*Sigh*