Re: [Qemu-devel] [PATCH v3 15/49] qapi: do not define enumeration value explicitely

[Marc-André Lureau]

Hi

On Fri, Jun 22, 2018 at 10:08 AM, Markus Armbruster <address@hidden> wrote:
> Subject: explicitly
>
> 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.
>>
>> 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 60c1d0a783..68a567f53f 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -2032,14 +2032,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;
>
> What excactly in your series depends on this?
>
> What safeguards do you propose to ensure an enumeration with
> conditionals is compiled only with the exact same conditionals within
> the same program?
>
> Example of the kind of deathtrap to guard against: compile
>
>     typedef enum Color {
>         COLOR_WHITE,
> #if defined(NEED_CPU_H)
> #if defined(TARGET_S390X)
>         COLOR_BLUE,
> #endif /* defined(TARGET_S390X) */
> #endif /* defined(NEED_CPU_H) */
>         COLOR_BLACK,
>     } Color;
>
> in s390x-code (COLOR_BLACK = 2) and in target-independent code
> (COLOR_BLACK = 1), then linking the two together.

This was a trick used in previous iterations. Now that we have a
separate target schema and generated code/headers, and since we have
poisoned identifiers, this should never happen.

> Yes, I know a similar deathtrap will be set up for struct and union
> types.  No excuse for ignoring either of the two.

Having gaps in the enums makes it harder to iterate over all values,
and we use more memory than necessary when allocating based on MAX
value.

It's not a big problem, but I consider this more important than this
artificially made up broken build example.

-- 
Marc-André Lureau