Hi
On Wed, Dec 5, 2018 at 5:19 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.
>
> 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 <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 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.