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 ?
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 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
