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

[Marc-André Lureau]

Hi

On Fri, Dec 8, 2017 at 8:50 AM, Markus Armbruster <address@hidden> wrote:
> 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.

What problem appears in patch 16? Some code could be introduced using
NEED_CPU_H and link arch & independent code together? It is still true
after patch 44. If necessary, I can work on a split-qapi series before
the conditionals are added. But the real benefit is only apparent
after the conditional are introduced, so I am not motivated to
reorder.

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

thanks

-- 
Marc-André Lureau