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

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> 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

Looking... aha!  target.json appears in PATCH 44 (which I haven't even
glanced at, yet).  The problem appears in PATCH 16, though.  Perhaps a
bit of patch reshuffling would do.

>> 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).

It's a beefy series, and it's probably best to let me review the largest
prefix I can before we dive into discussion.