[PATCH v2 0/3] qapi: Simplify enum generation

[PATCH v2 0/3] qapi: Simplify enum generation

[Philippe Mathieu-Daudé]

QAPI generating enum count as part of the enum forces handling
impossible switch cases. Modify qapi/types.py to generate the
enum count as a definition.
Do not try to cover the unreachable 'default' case.
Clean files covering unreachable foo__MAX case.

Since v1:
- Update documentation (Markus)
- Do not generate empty enums (Markus)
- Collect R-b tags

Philippe Mathieu-Daudé (3):
  scripts/git.orderfile: Display QAPI script changes before schema ones
  qapi: Do not generate empty enum
  qapi: Generate enum count as definition

 docs/devel/qapi-code-gen.rst | 10 +++++-----
 scripts/qapi/schema.py       |  5 ++++-
 scripts/qapi/types.py        | 11 +++++++----
 scripts/qapi/visit.py        |  2 --
 audio/audio_template.h       |  3 ---
 audio/audio.c                |  6 ------
 migration/migration.c        |  2 --
 replay/replay-input.c        | 12 ------------
 softmmu/tpm-hmp-cmds.c       |  2 --
 ui/input-linux.c             |  4 ----
 ui/input.c                   |  6 ------
 scripts/git.orderfile        |  2 ++
 12 files changed, 18 insertions(+), 47 deletions(-)

-- 
2.38.1

[PATCH v2 1/3] scripts/git.orderfile: Display QAPI script changes before schema ones

[Philippe Mathieu-Daudé]

When modifying QAPI scripts and modifying C files along,
it makes sense to display QAPI changes first.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 scripts/git.orderfile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/git.orderfile b/scripts/git.orderfile
index 8edac0380b..70adc1a74a 100644
--- a/scripts/git.orderfile
+++ b/scripts/git.orderfile
@@ -22,6 +22,8 @@ Makefile*
 *.mak
 meson.build
 
+# qapi scripts
+scripts/qapi*
 # qapi schema
 qapi/*.json
 qga/*.json
-- 
2.38.1

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

[Philippe Mathieu-Daudé]

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/schema.py       | 5 ++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

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/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
-- 
2.38.1

[PATCH v2 3/3] qapi: Generate enum count as definition

[Philippe Mathieu-Daudé]

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;

When iterating over a QAPISchemaEnumType, all possible
values are covered. The 'default' switch case generated in
gen_visit_object_members() is now unreachable, remove 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
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/devel/qapi-code-gen.rst |  4 ++--
 scripts/qapi/types.py        | 11 +++++++----
 scripts/qapi/visit.py        |  2 --
 audio/audio_template.h       |  3 ---
 audio/audio.c                |  6 ------
 migration/migration.c        |  2 --
 replay/replay-input.c        | 12 ------------
 softmmu/tpm-hmp-cmds.c       |  2 --
 ui/input-linux.c             |  4 ----
 ui/input.c                   |  6 ------
 10 files changed, 9 insertions(+), 43 deletions(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index d684c7c24d..45b0da448d 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -227,7 +227,7 @@ optional 'prefix' member overrides PREFIX.
 
 The generated C enumeration constants have values 0, 1, ..., N-1 (in
 QAPI schema order), where N is the number of values.  There is an
-additional enumeration constant PREFIX__MAX with value N.
+additional definition constant PREFIX__MAX with value N.
 
 Do not use string or an integer type when an enumeration type can do
 the job satisfactorily.
@@ -1825,7 +1825,7 @@ Example::
 
     typedef enum example_QAPIEvent {
         EXAMPLE_QAPI_EVENT_MY_EVENT,
-        EXAMPLE_QAPI_EVENT__MAX,
+    #define EXAMPLE_QAPI_EVENT__MAX 1
     } example_QAPIEvent;
 
     #define example_QAPIEvent_str(val) \
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/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 26a584ee4c..f66a31a963 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -159,8 +159,6 @@ def gen_visit_object_members(name: str,
 
             ret += var.ifcond.gen_endif()
         ret += mcgen('''
-    default:
-        abort();
     }
 ''')
 
diff --git a/audio/audio_template.h b/audio/audio_template.h
index e42326c20d..d545c03afb 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -376,9 +376,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/audio/audio.c b/audio/audio.c
index 70b096713c..ea372288eb 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2071,9 +2071,6 @@ void audio_create_pdos(Audiodev *dev)
         CASE(SPICE, spice, );
 #endif
         CASE(WAV, wav, );
-
-    case AUDIODEV_DRIVER__MAX:
-        abort();
     };
 }
 
@@ -2219,9 +2216,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/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/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;
     }
 }
 
-- 
2.38.1

Re: [PATCH v2 0/3] qapi: Simplify enum generation

[Philippe Mathieu-Daudé]

On 15/3/23 12:13, Philippe Mathieu-Daudé wrote:
> QAPI generating enum count as part of the enum forces handling
> impossible switch cases. Modify qapi/types.py to generate the
> enum count as a definition.
> Do not try to cover the unreachable 'default' case.
> Clean files covering unreachable foo__MAX case.
> 
> Since v1:
> - Update documentation (Markus)
> - Do not generate empty enums (Markus)
> - Collect R-b tags
> 
> Philippe Mathieu-Daudé (3):
>    scripts/git.orderfile: Display QAPI script changes before schema ones
>    qapi: Do not generate empty enum

Wrong branch... v3 coming.

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

[Stefan Berger]

On 3/15/23 07:13, 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/schema.py       | 5 ++++-
>   2 files changed, 7 insertions(+), 4 deletions(-)
> 
> 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/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

not: assert isinstance(members, list)

>           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


Acked-by: Stefan Berger <stefanb@linux.ibm.com>

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

[Philippe Mathieu-Daudé]

On 15/3/23 16:19, Stefan Berger wrote:
> On 3/15/23 07:13, 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/schema.py       | 5 ++++-
>>   2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> 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/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
> 
> not: assert isinstance(members, list)

This doesn't work as [] is a list instance (we want to check the
members[] array contains elements). More verbose could be:

             assert len(members) > 0

>>           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
> 
> 
> Acked-by: Stefan Berger <stefanb@linux.ibm.com>

Thanks!