Re: [Qemu-devel] [PATCH v6 06/27] qapi: do not define enumeration value explicitly

[Marc-André Lureau]

Hi
On Wed, Dec 5, 2018 at 5:19 PM Markus Armbruster <address@hidden> wrote:
>
> Marc-André Lureau <address@hidden> 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.
>
> Yes, gaps can be annoying, e.g. in lookup tables where gaps get confused
> with sentinels.
>
> Avoiding gaps scares me.  You have to religiously compile the enum with
> the exact same configuration everywhere in the program, or else you end
> up with bugs that are hard to spot.  Therefore:
>
> > Whenever config-host.h is changed, all the enum/types are recompiled.
> >
> > (a subsequent patch will split the schema. Target-specific poisoined
> > conditionals will be added. They cannot be mixed with the common
> > schema: it is not possible to end up with enums of different values in
> > common and target builds)
>
> Should we mention config-target.h here?
>

My memory isn't clear. I'll drop that comment from here, we will
discuss the poisoned conditionals again with the patch splitting the
schema.

> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  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 02c5c6767a..6f9498566e 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;
>
> I need to consider the whole series to decide whether avoiding gaps is a
> good idea.  If it is, then this patch is fine.