Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[Jeff Law <law at redhat dot com>]

On 11/02/2015 12:35 PM, David Malcolm wrote:

> > diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> > index fff4862..2559a36 100644
> > --- a/gdb/ada-lang.c
> > +++ b/gdb/ada-lang.c
> > @@ -11359,9 +11359,11 @@ ada_evaluate_subexp (struct type *expect_type, struct expression *exp,
> >              return value_zero (ada_aligned_type (type), lval_memory);
> >            }
> >          else
> > -        arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0);
> > -        arg1 = unwrap_value (arg1);
> > -        return ada_to_fixed_value (arg1);
> > +	{
> > +	  arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0);
> > +	  arg1 = unwrap_value (arg1);
> > +	  return ada_to_fixed_value (arg1);
> > +	}
> >
> >        case OP_TYPE:
> >          /* The value is not supposed to be used.  This is here to make it
>
> Interesting.  It's not technically a bug, since the "if true" clause has
> an unconditional return, but it looks like a time-bomb to me.  I'm happy
> that we warn for it.
Agreed.


> These three are all of the form:
>     if (record_debug)
>       fprint (...multiple lines...);
>       return -1;
>
> It seems reasonable to me to complain about the indentation of the
> return -1;
Agreed.


> > diff --git a/sysdeps/ieee754/flt-32/k_rem_pio2f.c b/sysdeps/ieee754/flt-32/k_rem_pio2f.c
> > index 0c7685c..a0d844c 100644
> > --- a/sysdeps/ieee754/flt-32/k_rem_pio2f.c
> > +++ b/sysdeps/ieee754/flt-32/k_rem_pio2f.c
> > @@ -65,7 +65,8 @@ int __kernel_rem_pio2f(float *x, float *y, int e0, int nx, int prec, const int32
> >
> >        /* compute q[0],q[1],...q[jk] */
> >           for (i=0;i<=jk;i++) {
> > -           for(j=0,fw=0.0;j<=jx;j++) fw += x[j]*f[jx+i-j]; q[i] = fw;
> > +           for(j=0,fw=0.0;j<=jx;j++) fw += x[j]*f[jx+i-j];
> > +           q[i] = fw;
> >           }
> >
> >           jz = jk;
>
> I think it's very reasonable to complain about the above code.
Agreed.

> So if I've counted things right the tally is:
> * 5 dubious-looking though correct places, where it's reasonable to
> issue a warning
> * 5 places where there's a blank line between guarded and non-guarded
> stmt, where patch 1 of the kit would have suppressed the warning
> * one bug (PR 68187)
>
> I think we want the first kind of thing at -Wall, but I'm not so keen on
> the second kind at -Wall.  Is there precedent for "levels" of a warning?
> (so e.g. pedantry level 1 at -Wall, level 2 at -Wextra, and have patch 1
> be the difference between levels 1 and 2?)
>
> Sorry for harping on about patch 1; thanks again for posting this
No worries about harping.  These are real world cases.

And yes, there's definitely precedent for different levels of a warning. 
 If you wanted to add a patch to have different levels and select one 
for -Wall, I'd look favorably on that.

Jeff