[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 15/49] qapi: do not define enumeration value
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3 15/49] qapi: do not define enumeration value explicitely |
Date: |
Thu, 28 Jun 2018 08:34:29 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> Hi
>
> On Fri, Jun 22, 2018 at 10:08 AM, Markus Armbruster <address@hidden> wrote:
>> Subject: explicitly
>>
>> 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/common.py | 7 ++-----
>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>> index 60c1d0a783..68a567f53f 100644
>>> --- a/scripts/qapi/common.py
>>> +++ b/scripts/qapi/common.py
>>> @@ -2032,14 +2032,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;
>>
>> What excactly in your series depends on this?
>>
>> What safeguards do you propose to ensure an enumeration with
>> conditionals is compiled only with the exact same conditionals within
>> the same program?
>>
>> Example of the kind of deathtrap to guard against: compile
>>
>> 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.
>
> This was a trick used in previous iterations. Now that we have a
> separate target schema and generated code/headers, and since we have
> poisoned identifiers, this should never happen.
>
>> Yes, I know a similar deathtrap will be set up for struct and union
>> types. No excuse for ignoring either of the two.
>
> Having gaps in the enums makes it harder to iterate over all values,
> and we use more memory than necessary when allocating based on MAX
> value.
>
> It's not a big problem, but I consider this more important than this
> artificially made up broken build example.
Made up yes, but the making up comes from painful experience debugging
real programs :)
I'm not at all opposed to "contracting" enums. I just want to be
persuaded it's safe. I think your argument is "Now that we have a
separate target schema and generated code/headers, and since we have
poisoned identifiers, this should never happen." Can you elaborate?
If that goes as I expect it to go, the next request will be "now put
that into your commit message" :)