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

[Daniel P . Berrangé]

On Thu, Mar 16, 2023 at 03:39:59PM +0100, Juan Quintela wrote:
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> > 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
> >> > 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.
> 
> Do we have an example for this?
> Because it looks really weird.  I would expect that the "container" unit
> of that enumeration is #ifdef out of compilation somehow.

I'm not sure if such an example physically exists. I know the  audio
code gets close, with all but 2 options conditional:

{ 'enum': 'AudiodevDriver',
  'data': [ 'none',
            { 'name': 'alsa', 'if': 'CONFIG_AUDIO_ALSA' },
            { 'name': 'coreaudio', 'if': 'CONFIG_AUDIO_COREAUDIO' },
            { 'name': 'dbus', 'if': 'CONFIG_DBUS_DISPLAY' },
            { 'name': 'dsound', 'if': 'CONFIG_AUDIO_DSOUND' },
            { 'name': 'jack', 'if': 'CONFIG_AUDIO_JACK' },
            { 'name': 'oss', 'if': 'CONFIG_AUDIO_OSS' },
            { 'name': 'pa', 'if': 'CONFIG_AUDIO_PA' },
            { 'name': 'sdl', 'if': 'CONFIG_AUDIO_SDL' },
            { 'name': 'sndio', 'if': 'CONFIG_AUDIO_SNDIO' },
            { 'name': 'spice', 'if': 'CONFIG_SPICE' },
            'wav' ] }

Just wanted to warn that we shouldn't assume empty enums can't
exist, because it would be quite easy to add 2 extra conditionals
to this audio example, and the enum wouldn't appear empty at a
glance, but none the less could be empty in some compile scenarios

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|