| Message ID | 20230315112811.22355-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: Simplify enum generation | expand |
On 3/15/23 04:28, Philippe Mathieu-Daudé wrote: > 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> > --- > docs/devel/qapi-code-gen.rst | 6 +++--- > scripts/qapi/events.py | 2 ++ > scripts/qapi/schema.py | 5 ++++- > scripts/qapi/types.py | 2 ++ > 4 files changed, 11 insertions(+), 4 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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.
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. With regards, Daniel
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. Later, Juan.
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
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*
On 16/3/23 15:57, Markus Armbruster wrote: > 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 :) I can't find how empty enums are considered by the C standard... > 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. Good catch... Using: -- >8 -- diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py @@ -110,8 +110,11 @@ def gen_enum(name: str, ret += mcgen(''' } %(c_name)s; +_Static_assert(%(c_last_enum)s == %(c_length)s - 1, "%(c_name)s"); ''', - c_name=c_name(name)) + c_name=c_name(name), + c_last_enum=c_enum_const(name, members[-1].name, prefix), + c_length=len(members)) ret += mcgen(''' --- I get: ./qapi/qapi-types-block-core.h:355:1: error: static_assert failed due to requirement 'BLOCKDEV_AIO_OPTIONS_NATIVE == 3 - 1' "BlockdevAioOptions" _Static_assert(BLOCKDEV_AIO_OPTIONS_IO_URING == 3 - 1, "BlockdevAioOptions"); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./qapi/qapi-types-block-core.h:430:1: error: static_assert failed due to requirement 'BLOCKDEV_DRIVER_VVFAT == 47 - 1' "BlockdevDriver" _Static_assert(BLOCKDEV_DRIVER_VVFAT == 47 - 1, "BlockdevDriver"); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./qapi/qapi-types-char.h:110:1: error: static_assert failed due to requirement 'CHARDEV_BACKEND_KIND_MEMORY == 22 - 1' "ChardevBackendKind" _Static_assert(CHARDEV_BACKEND_KIND_MEMORY == 22 - 1, "ChardevBackendKind"); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ...
On 16/3/23 15:57, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > 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. Can we meet half-way only generating the MAX definitions for unconditional enums, keeping the conditional ones as is? -- >8 -- diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py @@ -88,16 +88,18 @@ def gen_enum(name: str, members: List[QAPISchemaEnumMember], prefix: Optional[str] = None) -> str: assert members - # append automatically generated _MAX value - enum_members = members + [QAPISchemaEnumMember('_MAX', None)] - ret = mcgen(''' typedef enum %(c_name)s { ''', c_name=c_name(name)) - for memb in enum_members: + has_cond = any(memb.ifcond.is_present() for memb in members) + if has_cond: + # append automatically generated _MAX value + members += [QAPISchemaEnumMember('_MAX', None)] + + for memb in members: ret += memb.ifcond.gen_if() ret += mcgen(''' %(c_enum)s, @@ -105,6 +107,13 @@ def gen_enum(name: str, c_enum=c_enum_const(name, memb.name, prefix)) ret += memb.ifcond.gen_endif() + if not has_cond: + ret += mcgen(''' +#define %(c_name)s %(c_length)s +''', + c_name=c_enum_const(name, '_MAX', prefix), + c_length=len(members)) + ret += mcgen(''' } %(c_name)s; ''', ---
On Tue, Mar 21, 2023 at 03:31:56PM +0100, Philippe Mathieu-Daudé wrote: > On 16/3/23 15:57, Markus Armbruster wrote: > > 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 :) > > I can't find how empty enums are considered by the C standard... The C standard doesn't really matter either. What we actually care about is whether GCC and CLang consider the empty enums to be permissible or not. or to put it another way... if it compiles, ship it :-) With regards, Daniel
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > Can we meet half-way only generating the MAX definitions for > unconditional enums, keeping the conditional ones as is? > > -- >8 -- > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py > @@ -88,16 +88,18 @@ def gen_enum(name: str, > members: List[QAPISchemaEnumMember], > prefix: Optional[str] = None) -> str: > assert members > - # append automatically generated _MAX value > - enum_members = members + [QAPISchemaEnumMember('_MAX', None)] > - > ret = mcgen(''' > > typedef enum %(c_name)s { > ''', > c_name=c_name(name)) > > - for memb in enum_members: > + has_cond = any(memb.ifcond.is_present() for memb in members) > + if has_cond: > + # append automatically generated _MAX value > + members += [QAPISchemaEnumMember('_MAX', None)] > + > + for memb in members: > ret += memb.ifcond.gen_if() > ret += mcgen(''' > %(c_enum)s, > @@ -105,6 +107,13 @@ def gen_enum(name: str, > c_enum=c_enum_const(name, memb.name, prefix)) > ret += memb.ifcond.gen_endif() > > + if not has_cond: > + ret += mcgen(''' > +#define %(c_name)s %(c_length)s > +''', > + c_name=c_enum_const(name, '_MAX', prefix), > + c_length=len(members)) > + > ret += mcgen(''' > } %(c_name)s; > ''', > --- I doubt the benefit "we need a silly case FOO__MAX only sometimes" is worth the special case. We could generate something like #if [last_member's condition] #define FOO__MAX (FOO_last_member + 1) #elif [second_to_last_member's condition] #define FOO__MAX (FOO_second_to_last_member + 1) ... #else #define FOO__MAX (FOO_last_unconditional_member + 1) #endif but whether that is worth the additional complexity seems doubtful, too.
On Tue, Mar 21, 2023 at 03:19:28PM +0000, Daniel P. Berrangé wrote: > On Tue, Mar 21, 2023 at 03:31:56PM +0100, Philippe Mathieu-Daudé wrote: > > On 16/3/23 15:57, Markus Armbruster wrote: > > > 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 :) > > > > I can't find how empty enums are considered by the C standard... > > The C standard doesn't really matter either. > > What we actually care about is whether GCC and CLang consider the > empty enums to be permissible or not. or to put it another way... > if it compiles, ship it :-) But it doesn't compile: $ cat foo.c typedef enum Blah { } Blah; int main(void) { Blah b = 0; } $ gcc -o foo -Wall foo.c foo.c:2:1: error: empty enum is invalid 2 | } Blah; | ^ foo.c: In function ‘main’: foo.c:4:5: error: unknown type name ‘Blah’; use ‘enum’ keyword to refer to the type 4 | Blah b = 0; | ^~~~ | enum foo.c:4:10: warning: unused variable ‘b’ [-Wunused-variable] 4 | Blah b = 0; | ^ So we _do_ need to avoid creating an enum with all members optional in the configuration where all options are disabled, if we want that configuration to compile. Or require that all QAPI enums have at least one non-optional member.
Eric Blake <eblake@redhat.com> writes: > On Tue, Mar 21, 2023 at 03:19:28PM +0000, Daniel P. Berrangé wrote: >> On Tue, Mar 21, 2023 at 03:31:56PM +0100, Philippe Mathieu-Daudé wrote: >> > On 16/3/23 15:57, Markus Armbruster wrote: >> > > 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 :) >> > >> > I can't find how empty enums are considered by the C standard... C99 §6.7.2.2 Enumeration specifiers enum-specifier: enum identifier-opt { enumerator-list } enum identifier-opt { enumerator-list , } enum identifier enumerator-list: enumerator enumerator-list , enumerator enumerator: enumeration-constant enumeration-constant = constant-expr Empty enum is a syntax error. >> The C standard doesn't really matter either. >> >> What we actually care about is whether GCC and CLang consider the >> empty enums to be permissible or not. or to put it another way... >> if it compiles, ship it :-) > > But it doesn't compile: > > $ cat foo.c > typedef enum Blah { > } Blah; > int main(void) { > Blah b = 0; > } > $ gcc -o foo -Wall foo.c > foo.c:2:1: error: empty enum is invalid > 2 | } Blah; > | ^ Gcc opts for an error message more useful than "identifier expected". > foo.c: In function ‘main’: > foo.c:4:5: error: unknown type name ‘Blah’; use ‘enum’ keyword to refer to the type > 4 | Blah b = 0; > | ^~~~ > | enum > foo.c:4:10: warning: unused variable ‘b’ [-Wunused-variable] > 4 | Blah b = 0; > | ^ > > So we _do_ need to avoid creating an enum with all members optional in > the configuration where all options are disabled, if we want that > configuration to compile. Or require that all QAPI enums have at > least one non-optional member. There is just one way to avoid creating such an enum: make sure the QAPI enum has members in all configurations we care for. The issue at hand is whether catching empty enums in qapi-gen already is practical. Desirable, because qapi-gen errors are friendlier than C compiler errors in generated code. Note "practical": qapi-gen makes an effort to catch errors before the C compiler catches them. But catching all of them is impractical. Having qapi-gen catch empty enums sure is practical for truly empty enums. But I doubt an enum without any members is a mistake people make much. It isn't for enums with only conditional members: the configuration that makes them all vanish may or may not actually matter, or even exist, and qapi-gen can't tell. The C compiler can tell, but only for the configuration being compiled. Requiring at least one non-optional member is a restriction: enums with only conditional members are now rejected even when the configuration that makes them all vanish does not actually matter. Is shielding the user from C compiler errors about empty enums worth the restriction?
diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst index 23e7f2fb1c..d684c7c24d 100644 --- a/docs/devel/qapi-code-gen.rst +++ b/docs/devel/qapi-code-gen.rst @@ -206,6 +206,9 @@ Syntax:: Member 'enum' names the enum type. +Empty enumeration (no member) does not generate anything (not even +constant PREFIX__MAX). + Each member of the 'data' array defines a value of the enumeration type. The form STRING is shorthand for :code:`{ 'name': STRING }`. The 'name' values must be be distinct. @@ -214,9 +217,6 @@ Example:: { 'enum': 'MyEnum', 'data': [ 'value1', 'value2', 'value3' ] } -Nothing prevents an empty enumeration, although it is probably not -useful. - On the wire, an enumeration type's value is represented by its (string) name. In C, it's represented by an enumeration constant. These are of the form PREFIX_NAME, where PREFIX is derived from the diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py index 3cf01e96b6..48f4ca9613 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -207,6 +207,8 @@ def _begin_user_module(self, name: str) -> None: def visit_end(self) -> None: self._add_module('./emit', ' * QAPI Events emission') + if not self._event_enum_members: + return self._genc.preamble_add(mcgen(''' #include "qemu/osdep.h" #include "%(prefix)sqapi-emit-events.h" diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 207e4d71f3..28045dbe93 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -309,6 +309,7 @@ class QAPISchemaEnumType(QAPISchemaType): def __init__(self, name, info, doc, ifcond, features, members, prefix): super().__init__(name, info, doc, ifcond, features) + assert members for m in members: assert isinstance(m, QAPISchemaEnumMember) m.set_defined_in(name) @@ -1047,8 +1048,10 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members): return name def _def_enum_type(self, expr: QAPIExpression): - name = expr['enum'] data = expr['data'] + if not data: + return + name = expr['enum'] prefix = expr.get('prefix') ifcond = QAPISchemaIfCond(expr.get('if')) info = expr.info diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index c39d054d2c..7a7be7315f 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -42,6 +42,7 @@ def gen_enum_lookup(name: str, members: List[QAPISchemaEnumMember], prefix: Optional[str] = None) -> str: + assert members max_index = c_enum_const(name, '_MAX', prefix) feats = '' ret = mcgen(''' @@ -86,6 +87,7 @@ def gen_enum_lookup(name: str, def gen_enum(name: str, members: List[QAPISchemaEnumMember], prefix: Optional[str] = None) -> str: + assert members # append automatically generated _MAX value enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
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> --- docs/devel/qapi-code-gen.rst | 6 +++--- scripts/qapi/events.py | 2 ++ scripts/qapi/schema.py | 5 ++++- scripts/qapi/types.py | 2 ++ 4 files changed, 11 insertions(+), 4 deletions(-)