[PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall
On Mon, 2015-11-02 at 16:41 -0700, Jeff Law wrote:
> 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, >elts[pc + 2].string, 0);
> >> -arg1 = unwrap_value (arg1);
> >> -return ada_to_fixed_value (arg1);
> >> + {
> >> +arg1 = ada_value_struct_elt (arg1, >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.
The attached patch implements this idea.
It's a revised version of:
"[PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely
blank lines"
(https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03225.html)
(which wasn't approved, since you *did* want warnings for them),
and of:
"[PATCH 4/4] Add -Wmisleading-indentation to -Wall"
(https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03223.html)
which you approved, but which Richi was unhappy with
(I haven't committed it).
The attached patch adds the idea of a strictness level to
-Wmisleading-indentation, where
-Wmisleading-indentation
becomes equivalent to
-Wmisleading-indentation=1
The heuristic from patch 1 becomes the difference between
-Wmisleading-indentation=1 and -Wmisleading-indentation=2.
It adds -Wmisleading-indentation=1 to -Wall, with
-Wmisleading-indentation=2 available for users who want
extra strictness.
I think this gives us a big improvement in the signal:noise ratio of
the "-Wall" form of the warning for real code (see e.g. the stats for
gcc itself above), relative to the earlier form of the -Wall patch.
Bootstrapped with x86_64-pc-linux-gnu.
Adds 33 PASS results to g++.sum; adds 11 PASS results to gcc.sum.
OK for trunk?
gcc/c-family/ChangeLog:
* c-indentation.c (line_is_blank_p): New function.
(separated_by_blank_lines_p): New function.
(should_warn_for_misleading_indentation): Add param
strictness_level. Move early reject from
warn_for_misleading_indentation to here. At strictness
level 1, don't warn if the guarded and unguarded code are
separated by one or more entirely blank lines.
(warn_for_misleading_indentation): Pass value of
warn_misleading_indentation to
should_warn_for_misleading_indentation and move early
rejection case for disabled aka level 0 to there.
* c.opt (Wmisleading-indentation): Convert to an alias for...
(Wmisleading-indentation=): ...this new version of
Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall
On 12/09/2015 04:38 PM, David Malcolm wrote:
+/* The following function contains examples of bad indentation that's
+ arguably not misleading, due to a blank line between the guarded and the
+ non-guarded code. Some of the blank lines deliberately contain
+ redundant whitespace, to verify that this is handled.
+ Based on examples seen in gcc/fortran/io.c, gcc/function.c, and
+ gcc/regrename.c.
+
+ At -Wmisleading-indentation (implying level 1) we shouldn't emit
+ warnings for this function. Compare with
+ fn_40_level_1 in Wmisleading-indentation-level-1.c and
+ fn_40_level_2 in Wmisleading-indentation-level-2.c. */
+
+void
+fn_40_implicit_level_1 (int arg)
+{
+if (flagA)
+ foo (0);
+
+ foo (1);
+
I personally see no use for the blank line heuristic, in fact I think it
is misguided. To my eyes a warning is clearly warranted for the examples
in this testcase. Do we actually have any users calling for this heuristic?
Bernd
Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall
On Wed, 2015-12-09 at 16:40 +0100, Bernd Schmidt wrote:
> On 12/09/2015 04:38 PM, David Malcolm wrote:
> > +/* The following function contains examples of bad indentation that's
> > + arguably not misleading, due to a blank line between the guarded and the
> > + non-guarded code. Some of the blank lines deliberately contain
> > + redundant whitespace, to verify that this is handled.
> > + Based on examples seen in gcc/fortran/io.c, gcc/function.c, and
> > + gcc/regrename.c.
> > +
> > + At -Wmisleading-indentation (implying level 1) we shouldn't emit
> > + warnings for this function. Compare with
> > + fn_40_level_1 in Wmisleading-indentation-level-1.c and
> > + fn_40_level_2 in Wmisleading-indentation-level-2.c. */
> > +
> > +void
> > +fn_40_implicit_level_1 (int arg)
> > +{
> > +if (flagA)
> > + foo (0);
> > +
> > + foo (1);
> > +
>
> I personally see no use for the blank line heuristic, in fact I think it
> is misguided. To my eyes a warning is clearly warranted for the examples
> in this testcase.
This goes back to the examples given in:
https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03225.html
This is about managing the signal:noise ratio for something in -Wall.
The distinction I want to make here is between badly indented code vs
misleadingly indented code. Yes, the code is badly indented, but to my
eyes the code is sufficiently badly indented that it has become *not*
misleading: I can immediately see that something is awry. I want to
preserve the -Wall level of the warning for the cases when it's not
immediately clear that there's a problem.
The levels idea means that you can turn off the blank-lines heuristic by
setting -Wmisleading-indentation=2.
> Do we actually have any users calling for this heuristic?
FWIW, gcc trunk is clean with the blank-lines heuristic, but not without
it (see the URL above for the 3 places I know of that fire without the
heuristic).
Dave
Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall
On 12/09/2015 05:49 PM, David Malcolm wrote:
+void
+fn_40_implicit_level_1 (int arg)
+{
+if (flagA)
+ foo (0);
+
+ foo (1);
+
The distinction I want to make here is between badly indented code vs
misleadingly indented code. Yes, the code is badly indented, but to my
eyes the code is sufficiently badly indented that it has become *not*
misleading: I can immediately see that something is awry.
To my eyes and brain the effect of this code is disconcerting to the
point of confusion (and there was a case recently where a patch was
posted with a similar problem and it initially made me misinterpret the
code). So - if a human had seen this they'd have fixed it. Code like
this could have come in through a misapplied patch for example, and
you'd want to warn in such a case.
Do we actually have any users calling for this heuristic?
FWIW, gcc trunk is clean with the blank-lines heuristic, but not without
it (see the URL above for the 3 places I know of that fire without the
heuristic).
That's a fairly low false-positive rate if we even want to call it that.
I'd fix the three examples and leave the warning code as is. We can
revisit the decision if we get a flood of bug reports.
Bernd
Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall
On 12/09/2015 04:49 PM, David Malcolm wrote: > This is about managing the signal:noise ratio for something in -Wall. > > The distinction I want to make here is between badly indented code vs > misleadingly indented code. Yes, the code is badly indented, but to my > eyes the code is sufficiently badly indented that it has become *not* > misleading: I can immediately see that something is awry. I want to > preserve the -Wall level of the warning for the cases when it's not > immediately clear that there's a problem. > > The levels idea means that you can turn off the blank-lines heuristic by > setting -Wmisleading-indentation=2. IMHO, the problem with the levels idea will be if/when later you come up with some other orthogonal heuristic to catch some other class of indentation problem, and users want to enable it but not the blank-lines heuristic, or vice-versa. Also, levels don't allow finer-selection with "-Werror -Wno-error=misleading-indentation", IIUC. Did you consider -Wmisleading-indentation=blank-lines,foo,bar instead, or totally separate switches like: -Wmisleading-indentation -Wmisleading-indentation-blank-lines -Wmisleading-indentation-foo -Wmisleading-indentation-bar ?
