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

[PATCH v3 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 v2:
- Post correct branch generating empty foo-qapi-emit-events.h

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/events.py       |  2 ++
 scripts/qapi/schema.py       |  5 ++++-
 scripts/qapi/types.py        | 13 +++++++++----
 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 ++
 13 files changed, 22 insertions(+), 47 deletions(-)

-- 
2.38.1

[PATCH v3 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 v3 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/events.py       | 2 ++
 scripts/qapi/schema.py       | 5 ++++-
 scripts/qapi/types.py        | 2 ++
 4 files changed, 11 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/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)]
 
-- 
2.38.1

[PATCH v3 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 7a7be7315f..6459a6f925 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -88,16 +88,13 @@ 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:
+    for memb in members:
         ret += memb.ifcond.gen_if()
         ret += mcgen('''
     %(c_enum)s,
@@ -105,6 +102,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 v3 2/3] qapi: Do not generate empty enum

[Richard Henderson]

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~

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

[Philippe Mathieu-Daudé]

On 15/3/23 12:28, Philippe Mathieu-Daudé wrote:
> When modifying QAPI scripts and modifying C files along,
> it makes sense to display QAPI changes first.

Patch 3 of this series illustrates this (and has been
produced using this orderfile).

> 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

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

[Dr. David Alan Gilbert]

* 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;
> 
> 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 7a7be7315f..6459a6f925 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -88,16 +88,13 @@ 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:
> +    for memb in members:
>          ret += memb.ifcond.gen_if()
>          ret += mcgen('''
>      %(c_enum)s,
> @@ -105,6 +102,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();

Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>      }
>  
>      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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Juan Quintela]

Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 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>

Reviewed-by: Juan Quintela <quintela@redhat.com>

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

[Markus Armbruster]

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.

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

[Daniel P. Berrangé]

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
-- 
|: 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 :|

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

[Juan Quintela]

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.

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 :|

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*

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

[Philippe Mathieu-Daudé]

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");
^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...

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

[Philippe Mathieu-Daudé]

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;
  ''',
---

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

[Daniel P. Berrangé]

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
-- 
|: 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 :|

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

[Markus Armbruster]

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.

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

[Eric Blake]

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, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Markus Armbruster]

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?