On Thu, Dec 7, 2017 at 5:23 PM, Markus Armbruster <arm...@redhat.com> wrote:
> Marc-André Lureau <marcandre.lur...@redhat.com> 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 <marcandre.lur...@redhat.com>
>> ---
>> 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
