| Message ID | 20230224155451.20211-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: Simplify enum generation | expand |
On 2/24/23 05:54, Philippe Mathieu-Daudé wrote: > QAPI's gen_enum() generates QAPI enum values and the number > of this values (as foo__MAX). > The number of entries in an enum type is not part of the > enumerated values, but we generate it as such. See for > example: > > typedef enum OnOffAuto { > ON_OFF_AUTO_AUTO, > ON_OFF_AUTO_ON, > ON_OFF_AUTO_OFF, > ON_OFF_AUTO__MAX, <--------- > } OnOffAuto; > > Instead of declaring the enum count as the last enumerated > value, #define it, so it is not part of the enum. The previous > example becomes: > > typedef enum OnOffAuto { > ON_OFF_AUTO_AUTO, > ON_OFF_AUTO_ON, > ON_OFF_AUTO_OFF, > #define ON_OFF_AUTO__MAX 3 <--------- > } OnOffAuto; > > Since Clang enables the -Wswitch warning by default [*], remove all > pointless foo__MAX cases in switch statement, in order to avoid: > > audio/audio.c:2231:10: error: case value not in enumerated type 'AudioFormat' (aka 'enum AudioFormat') [-Wswitch] > case AUDIO_FORMAT__MAX: > ^ > ui/input.c:233:14: error: case value not in enumerated type 'KeyValueKind' (aka 'enum KeyValueKind') [-Wswitch] > case KEY_VALUE_KIND__MAX: > ^ > ... > > [*]https://clang.llvm.org/docs/DiagnosticsReference.html#wswitch > Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org> > --- Yay! Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > QAPI's gen_enum() generates QAPI enum values and the number > of this values (as foo__MAX). > The number of entries in an enum type is not part of the > enumerated values, but we generate it as such. See for > example: > > typedef enum OnOffAuto { > ON_OFF_AUTO_AUTO, > ON_OFF_AUTO_ON, > ON_OFF_AUTO_OFF, > ON_OFF_AUTO__MAX, <--------- > } OnOffAuto; > > Instead of declaring the enum count as the last enumerated > value, #define it, so it is not part of the enum. The previous > example becomes: > > typedef enum OnOffAuto { > ON_OFF_AUTO_AUTO, > ON_OFF_AUTO_ON, > ON_OFF_AUTO_OFF, > #define ON_OFF_AUTO__MAX 3 <--------- > } OnOffAuto; > > Since Clang enables the -Wswitch warning by default [*], remove all > pointless foo__MAX cases in switch statement, in order to avoid: > > audio/audio.c:2231:10: error: case value not in enumerated type 'AudioFormat' (aka 'enum AudioFormat') [-Wswitch] > case AUDIO_FORMAT__MAX: > ^ > ui/input.c:233:14: error: case value not in enumerated type 'KeyValueKind' (aka 'enum KeyValueKind') [-Wswitch] > case KEY_VALUE_KIND__MAX: > ^ > ... > > [*] https://clang.llvm.org/docs/DiagnosticsReference.html#wswitch > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Juan Quintela <quintela@redhat.com> This other is very nice. Thanks, Juan.
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > QAPI's gen_enum() generates QAPI enum values and the number > of this values (as foo__MAX). > The number of entries in an enum type is not part of the > enumerated values, but we generate it as such. See for > example: > > typedef enum OnOffAuto { > ON_OFF_AUTO_AUTO, > ON_OFF_AUTO_ON, > ON_OFF_AUTO_OFF, > ON_OFF_AUTO__MAX, <--------- > } OnOffAuto; > > Instead of declaring the enum count as the last enumerated > value, #define it, so it is not part of the enum. The previous > example becomes: > > typedef enum OnOffAuto { > ON_OFF_AUTO_AUTO, > ON_OFF_AUTO_ON, > ON_OFF_AUTO_OFF, > #define ON_OFF_AUTO__MAX 3 <--------- > } OnOffAuto; I'm in favour. In fact, I've wanted to do this for a while, just never got around to it. > Since Clang enables the -Wswitch warning by default [*], remove all > pointless foo__MAX cases in switch statement, in order to avoid: > > audio/audio.c:2231:10: error: case value not in enumerated type 'AudioFormat' (aka 'enum AudioFormat') [-Wswitch] > case AUDIO_FORMAT__MAX: > ^ > ui/input.c:233:14: error: case value not in enumerated type 'KeyValueKind' (aka 'enum KeyValueKind') [-Wswitch] > case KEY_VALUE_KIND__MAX: > ^ > ... > > [*] https://clang.llvm.org/docs/DiagnosticsReference.html#wswitch > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Falls apart when the enum is empty: gcc -m64 -mcx16 -Iqga/qemu-ga.p -Iqga -I../qga -I. -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -std=gnu11 -O0 -g -isystem /work/armbru/qemu/linux-headers -isystem linux-headers -iquote . -iquote /work/armbru/qemu -iquote /work/armbru/qemu/include -iquote /work/armbru/qemu/tcg/i386 -pthread -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -MD -MQ qga/qemu-ga.p/meson-generated_.._qga-qapi-emit-events.c.o -MF qga/qemu-ga.p/meson-generated_.._qga-qapi-emit-events.c.o.d -o qga/qemu-ga.p/meson-generated_.._qga-qapi-emit-events.c.o -c qga/qga-qapi-emit-events.c 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; | ^ qga/qapi-schema.json does not define any events, so its (implicitly defined) event enumeration comes out empty. We could detect this, and either emit a dummy event, or suppress code generation for events entirely. Explicitly defined enumerations may also be empty. qapi-code-gen.rst says: Nothing prevents an empty enumeration, although it is probably not useful. We could forbid empty enumerations. Speaking of qapi-code-gen.rst: it also needs an update. Search for __MAX.
On 27/2/23 14:10, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > >> QAPI's gen_enum() generates QAPI enum values and the number >> of this values (as foo__MAX). >> The number of entries in an enum type is not part of the >> enumerated values, but we generate it as such. See for >> example: >> >> typedef enum OnOffAuto { >> ON_OFF_AUTO_AUTO, >> ON_OFF_AUTO_ON, >> ON_OFF_AUTO_OFF, >> ON_OFF_AUTO__MAX, <--------- >> } OnOffAuto; >> >> Instead of declaring the enum count as the last enumerated >> value, #define it, so it is not part of the enum. The previous >> example becomes: >> >> typedef enum OnOffAuto { >> ON_OFF_AUTO_AUTO, >> ON_OFF_AUTO_ON, >> ON_OFF_AUTO_OFF, >> #define ON_OFF_AUTO__MAX 3 <--------- >> } OnOffAuto; > > I'm in favour. In fact, I've wanted to do this for a while, just never > got around to it. > >> Since Clang enables the -Wswitch warning by default [*], remove all >> pointless foo__MAX cases in switch statement, in order to avoid: >> >> audio/audio.c:2231:10: error: case value not in enumerated type 'AudioFormat' (aka 'enum AudioFormat') [-Wswitch] >> case AUDIO_FORMAT__MAX: >> ^ >> ui/input.c:233:14: error: case value not in enumerated type 'KeyValueKind' (aka 'enum KeyValueKind') [-Wswitch] >> case KEY_VALUE_KIND__MAX: >> ^ >> ... >> >> [*] https://clang.llvm.org/docs/DiagnosticsReference.html#wswitch >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Falls apart when the enum is empty: > > gcc -m64 -mcx16 -Iqga/qemu-ga.p -Iqga -I../qga -I. -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -std=gnu11 -O0 -g -isystem /work/armbru/qemu/linux-headers -isystem linux-headers -iquote . -iquote /work/armbru/qemu -iquote /work/armbru/qemu/include -iquote /work/armbru/qemu/tcg/i386 -pthread -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -MD -MQ qga/qemu-ga.p/meson-generated_.._qga-qapi-emit-events.c.o -MF qga/qemu-ga.p/meson-generated_.._qga-qapi-emit-events.c.o.d -o qga/qemu-ga.p/meson-generated_.._qga-qapi-emit-events.c.o -c qga/qga-qapi-emit-events.c > 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; > | ^ While I could find that in the C++ standard, I couldn't in the C one. I wonder if in that case QAPI should generate foo__MAX = 0. No code / structure should use an enum type which doesn't contain any member...
diff --git a/audio/audio.c b/audio/audio.c index 4290309d18..57130c44f6 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -2079,9 +2079,6 @@ void audio_create_pdos(Audiodev *dev) CASE(SPICE, spice, ); #endif CASE(WAV, wav, ); - - case AUDIODEV_DRIVER__MAX: - abort(); }; } @@ -2227,9 +2224,6 @@ int audioformat_bytes_per_sample(AudioFormat fmt) case AUDIO_FORMAT_S32: case AUDIO_FORMAT_F32: return 4; - - case AUDIO_FORMAT__MAX: - ; } abort(); } diff --git a/audio/audio_template.h b/audio/audio_template.h index 42b4712acb..58e1255d7a 100644 --- a/audio/audio_template.h +++ b/audio/audio_template.h @@ -369,9 +369,6 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, TYPE)(Audiodev *dev) #endif case AUDIODEV_DRIVER_WAV: return dev->u.wav.TYPE; - - case AUDIODEV_DRIVER__MAX: - break; } abort(); } diff --git a/migration/migration.c b/migration/migration.c index ae2025d9d8..bdadab3b5e 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2220,8 +2220,6 @@ bool migration_is_idle(void) case MIGRATION_STATUS_DEVICE: case MIGRATION_STATUS_WAIT_UNPLUG: return false; - case MIGRATION_STATUS__MAX: - g_assert_not_reached(); } return false; diff --git a/replay/replay-input.c b/replay/replay-input.c index 1147e3d34e..c6de8e33ac 100644 --- a/replay/replay-input.c +++ b/replay/replay-input.c @@ -38,9 +38,6 @@ void replay_save_input_event(InputEvent *evt) replay_put_dword(key->key->u.qcode.data); replay_put_byte(key->down); break; - case KEY_VALUE_KIND__MAX: - /* keep gcc happy */ - break; } break; case INPUT_EVENT_KIND_BTN: @@ -58,9 +55,6 @@ void replay_save_input_event(InputEvent *evt) replay_put_dword(move->axis); replay_put_qword(move->value); break; - case INPUT_EVENT_KIND__MAX: - /* keep gcc happy */ - break; } } @@ -89,9 +83,6 @@ InputEvent *replay_read_input_event(void) evt.u.key.data->key->u.qcode.data = (QKeyCode)replay_get_dword(); evt.u.key.data->down = replay_get_byte(); break; - case KEY_VALUE_KIND__MAX: - /* keep gcc happy */ - break; } break; case INPUT_EVENT_KIND_BTN: @@ -109,9 +100,6 @@ InputEvent *replay_read_input_event(void) evt.u.abs.data->axis = (InputAxis)replay_get_dword(); evt.u.abs.data->value = replay_get_qword(); break; - case INPUT_EVENT_KIND__MAX: - /* keep gcc happy */ - break; } return QAPI_CLONE(InputEvent, &evt); diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index c39d054d2c..b24bcb40ad 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -86,16 +86,13 @@ def gen_enum_lookup(name: str, def gen_enum(name: str, members: List[QAPISchemaEnumMember], prefix: Optional[str] = None) -> str: - # 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: + for memb in members: ret += memb.ifcond.gen_if() ret += mcgen(''' %(c_enum)s, @@ -103,6 +100,12 @@ def gen_enum(name: str, c_enum=c_enum_const(name, memb.name, prefix)) ret += memb.ifcond.gen_endif() + 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; ''', diff --git a/softmmu/tpm-hmp-cmds.c b/softmmu/tpm-hmp-cmds.c index 9ed6ad6c4d..5a354cf6ac 100644 --- a/softmmu/tpm-hmp-cmds.c +++ b/softmmu/tpm-hmp-cmds.c @@ -52,8 +52,6 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict) teo = ti->options->u.emulator.data; monitor_printf(mon, ",chardev=%s", teo->chardev); break; - case TPM_TYPE__MAX: - break; } monitor_printf(mon, "\n"); c++; diff --git a/ui/input-linux.c b/ui/input-linux.c index e572a2e905..a6e7574422 100644 --- a/ui/input-linux.c +++ b/ui/input-linux.c @@ -120,10 +120,6 @@ static bool input_linux_check_toggle(InputLinux *il) return (il->keydown[KEY_LEFTCTRL] || il->keydown[KEY_RIGHTCTRL]) && il->keydown[KEY_SCROLLLOCK]; - - case GRAB_TOGGLE_KEYS__MAX: - /* avoid gcc error */ - break; } return false; } diff --git a/ui/input.c b/ui/input.c index f2d1e7a3a7..ca8f49a403 100644 --- a/ui/input.c +++ b/ui/input.c @@ -230,9 +230,6 @@ static void qemu_input_event_trace(QemuConsole *src, InputEvent *evt) name = QKeyCode_str(key->key->u.qcode.data); trace_input_event_key_qcode(idx, name, key->down); break; - case KEY_VALUE_KIND__MAX: - /* keep gcc happy */ - break; } break; case INPUT_EVENT_KIND_BTN: @@ -250,9 +247,6 @@ static void qemu_input_event_trace(QemuConsole *src, InputEvent *evt) name = InputAxis_str(move->axis); trace_input_event_abs(idx, name, move->value); break; - case INPUT_EVENT_KIND__MAX: - /* keep gcc happy */ - break; } }
QAPI's gen_enum() generates QAPI enum values and the number of this values (as foo__MAX). The number of entries in an enum type is not part of the enumerated values, but we generate it as such. See for example: typedef enum OnOffAuto { ON_OFF_AUTO_AUTO, ON_OFF_AUTO_ON, ON_OFF_AUTO_OFF, ON_OFF_AUTO__MAX, <--------- } OnOffAuto; Instead of declaring the enum count as the last enumerated value, #define it, so it is not part of the enum. The previous example becomes: typedef enum OnOffAuto { ON_OFF_AUTO_AUTO, ON_OFF_AUTO_ON, ON_OFF_AUTO_OFF, #define ON_OFF_AUTO__MAX 3 <--------- } OnOffAuto; Since Clang enables the -Wswitch warning by default [*], remove all pointless foo__MAX cases in switch statement, in order to avoid: audio/audio.c:2231:10: error: case value not in enumerated type 'AudioFormat' (aka 'enum AudioFormat') [-Wswitch] case AUDIO_FORMAT__MAX: ^ ui/input.c:233:14: error: case value not in enumerated type 'KeyValueKind' (aka 'enum KeyValueKind') [-Wswitch] case KEY_VALUE_KIND__MAX: ^ ... [*] https://clang.llvm.org/docs/DiagnosticsReference.html#wswitch Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- audio/audio.c | 6 ------ audio/audio_template.h | 3 --- migration/migration.c | 2 -- replay/replay-input.c | 12 ------------ scripts/qapi/types.py | 11 +++++++---- softmmu/tpm-hmp-cmds.c | 2 -- ui/input-linux.c | 4 ---- ui/input.c | 6 ------ 8 files changed, 7 insertions(+), 39 deletions(-)