[Qemu-devel] [PATCH for-4.0 v7 02/27] qapi: do not define enumeration value explicitly

Marc-André Lureau posted 27 patches 7 years, 2 months ago
There is a newer version of this series
[Qemu-devel] [PATCH for-4.0 v7 02/27] qapi: do not define enumeration value explicitly
Posted by Marc-André Lureau 7 years, 2 months ago
The C standard has the initial value at 0 and the subsequent values
incremented by 1. No need to set this explicitely.

This will prevent from artificial "gaps" when compiling out some enum
values and having unnecessarily large MAX values & enums arrays, or
simplifying iterating over valid enum values.

Whenever config-host.h is changed, all the enum/types are recompiled.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/qapi/common.py | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 7b62a4c7b0..7ea0cf5139 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -2045,14 +2045,11 @@ typedef enum %(c_name)s {
 ''',
                 c_name=c_name(name))
 
-    i = 0
     for value in enum_values:
         ret += mcgen('''
-    %(c_enum)s = %(i)d,
+    %(c_enum)s,
 ''',
-                     c_enum=c_enum_const(name, value, prefix),
-                     i=i)
-        i += 1
+                     c_enum=c_enum_const(name, value, prefix))
 
     ret += mcgen('''
 } %(c_name)s;
-- 
2.20.0.rc1


Re: [Qemu-devel] [PATCH for-4.0 v7 02/27] qapi: do not define enumeration value explicitly
Posted by Markus Armbruster 7 years, 1 month ago
Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> The C standard has the initial value at 0 and the subsequent values
> incremented by 1. No need to set this explicitely.
>
> This will prevent from artificial "gaps" when compiling out some enum
> values and having unnecessarily large MAX values & enums arrays, or
> simplifying iterating over valid enum values.

Should mention that compiling out will only become possible later in
this series.

> Whenever config-host.h is changed, all the enum/types are recompiled.

This soundness argument is incomplete.  Yes, our coding conventions
ensure everything gets recompiled when config-host.h changes.  But
nothing stops people from using 'if' conditions that depend on more than
just config-host.h.

What about this:

  qapi: Do not define enumeration value explicitly

  The generated C enumeration types explicitly set the enumeration
  constants to 0, 1, 2, ...  That's exactly what you get when you don't
  supply values.

  Drop the explicit values.  No change now, but it will avoid gaps in
  the values when we later add support for 'if' conditions.  Avoiding
  such gaps will save us the trouble of changing the ENUM_lookup[]
  tables to work without a sentinel.

  We'll have to take care to ensure the headers required by the 'if'
  conditions get always included before the generated QAPI code.
  Fortunately, our convention to include "qemu/osdep.h" first in any .c
  ensures that's the case for our CONFIG_FOO macros.

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

With an improved commit message:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH for-4.0 v7 02/27] qapi: do not define enumeration value explicitly
Posted by Marc-André Lureau 7 years, 1 month ago
Hi

On Wed, Dec 12, 2018 at 12:52 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > The C standard has the initial value at 0 and the subsequent values
> > incremented by 1. No need to set this explicitely.
> >
> > This will prevent from artificial "gaps" when compiling out some enum
> > values and having unnecessarily large MAX values & enums arrays, or
> > simplifying iterating over valid enum values.
>
> Should mention that compiling out will only become possible later in
> this series.
>
> > Whenever config-host.h is changed, all the enum/types are recompiled.
>
> This soundness argument is incomplete.  Yes, our coding conventions
> ensure everything gets recompiled when config-host.h changes.  But
> nothing stops people from using 'if' conditions that depend on more than
> just config-host.h.
>
> What about this:
>
>   qapi: Do not define enumeration value explicitly
>
>   The generated C enumeration types explicitly set the enumeration
>   constants to 0, 1, 2, ...  That's exactly what you get when you don't
>   supply values.
>
>   Drop the explicit values.  No change now, but it will avoid gaps in
>   the values when we later add support for 'if' conditions.  Avoiding
>   such gaps will save us the trouble of changing the ENUM_lookup[]
>   tables to work without a sentinel.
>
>   We'll have to take care to ensure the headers required by the 'if'
>   conditions get always included before the generated QAPI code.
>   Fortunately, our convention to include "qemu/osdep.h" first in any .c
>   ensures that's the case for our CONFIG_FOO macros.
>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> With an improved commit message:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

ack, thanks!