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

[Marc-André Lureau]

On Thu, Dec 7, 2017 at 5:23 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.
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  scripts/qapi.py | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 94b735d8d6..074ee221a1 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -1985,14 +1985,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;
>
> Recapitulate review of v2: this risks entertaining mishaps like
> compiling this one
>
>     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.
>
> Same issue for struct members and such (previous patch).
>
> What's our story on preventing disaster here?
>
> In the long run, we want to split the generated code so that
> target-specific and target-independent code are separate, and each part
> is always compiled with consistent preprocessor symbols.  But I'm afraid
> that's not in the card right now.

Eh, I need to refresh my memories about that series, but I think
that's what I did in v3

It doesn't use the NEED_CPU_H trick. It has a seperate per-target target.json


>
> I therefore proposed the stupidest temporary stopgap that could possibly
> work: apply conditionals *only* to qmp-introspect.c, leave everything
> unconditional elsewhere.

I don't like that idea much and I don't think we need that
restriction, but I need to get back to that series on some point
(probably after you finish the review).

-- 
Marc-André Lureau