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

[Markus Armbruster]

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

> 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's been a while...

Generated headers using conditionals must include the headers providing
the symbols used in conditionals.  Not doing so is an open death trap.

PATCH 16 sets up the first instance of the death trap.  Or maybe it's
PATCH 13.

However, including these headers only becomes possible *after* you split
off the target-specific stuff in PATCH 44.

Do I make any sense?

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

Understand.

As a maintainer, I can ask for improvements, but the only lever I have
is saying no.  Which should be reserved for cases that are actually
wrong, or create inacceptable technical debt.  Temporary death traps
don't count as either.  For cases I merely hate, when asking doesn't
help, all I can do is do the work myself.  So I did:

    [PATCH RFC 00/21] Modularize generated QAPI code
    Message-Id: <address@hidden>

[...]