Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> 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
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.
