Re: [PATCH v2 09/36] tcg: Consolidate 3 bits into enum TCGTempKind

[Richard Henderson]

On 4/23/20 10:24 AM, Daniel P. Berrangé wrote:
> On Thu, Apr 23, 2020 at 08:40:10AM -0700, Richard Henderson wrote:
>> On 4/23/20 2:00 AM, Philippe Mathieu-Daudé wrote:
>>>>> @@ -1885,12 +1896,17 @@ static char *tcg_get_arg_str_ptr(TCGContext *s, 
>>>>> char *buf, int buf_size,
>>>>>  {
>>>>>      int idx = temp_idx(ts);
>>>>>
>>>>> -    if (ts->temp_global) {
>>>>> +    switch (ts->kind) {
>>>>> +    case TEMP_FIXED:
>>>>> +    case TEMP_GLOBAL:
>>>>>          pstrcpy(buf, buf_size, ts->name);
>>>>> -    } else if (ts->temp_local) {
>>>>> +        break;
>>>>> +    case TEMP_LOCAL:
>>>>>          snprintf(buf, buf_size, "loc%d", idx - s->nb_globals);
>>>>> -    } else {
>>>>> +        break;
>>>>> +    case TEMP_NORMAL:
>>>>>          snprintf(buf, buf_size, "tmp%d", idx - s->nb_globals);
>>>>> +        break;
>>>>>      }
>>>>
>>>> Hmm, why this switch doesn't have:
>>>>
>>>>         default:
>>>>             g_assert_not_reached();
>>>>
>>>> like the other ones?
>>>
>>> ... then all switch should have a default case, as noticed Aleksandar.
>>
>> There's a bit of a conflict between wanting to use -Werror -Wswitch, and 
>> making
>> sure every switch has a default.
>>
>> With the former, you get a compiler error of the form
>>
>> error: enumeration value ‘FOO’ not handled in switch
>>
>> which lets you easily find places that need adjustment enumerators are added.
> 
> FYI,  -Wswitch-enum can deal with this. This gives a warning about
> missing enum cases, even if there is a default statement:
> 
> [quote]
> '-Wswitch-enum'
>      Warn whenever a 'switch' statement has an index of enumerated type
>      and lacks a 'case' for one or more of the named codes of that
>      enumeration.  'case' labels outside the enumeration range also
>      provoke warnings when this option is used.  The only difference
>      between '-Wswitch' and this option is that this option gives a
>      warning about an omitted enumeration code even if there is a
>      'default' label.

This warning, IMO, is useless.

All you need is one enumeration with 100 elements -- e.g. TCGOp -- and you
certainly will not want to have to add all enumerators to every switch.


r~