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

[Daniel P . Berrangé]

On Thu, Apr 23, 2020 at 04:11:14PM -0700, Richard Henderson wrote:
> 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.

It depends how many of these exceptions you have. If there are only a
small handful of exceptions like this, then you can use Pragmas to
selectively disable the warning in those few cases, while still
benefitting from it across the rest of the code.  If there are alot
of such exceptions though, then I agree it is impractical.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|