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

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Fri, Jun 22, 2018 at 10:08 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Subject: explicitly
>>
>> Marc-André Lureau <marcandre.lureau@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.lureau@redhat.com>
>>> ---
>>>  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" :)