[PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall

[David Malcolm <dmalcolm at redhat dot com>]

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, &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.

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&regrtested 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
	-Wmisleading-indentation, which accepts an argument.
	Add level 1 to -Wall for C and C++.

gcc/ChangeLog:
	* doc/invoke.texi (Warning Options): Add -Wmisleading-indentation=n.
	(-Wall): Add -Wmisleading-indentation=1 to the list.
	(-Wmisleading-indentation): Add -Wmisleading-indentation=n and
	description of levels.  Update documentation to reflect
	being enabled by -Wall in C/C++.  Provide an example showing
	the difference between levels 1 and 2.

gcc/testsuite/ChangeLog:
	* c-c++-common/Wmisleading-indentation-in-Wall.c: New.
	* c-c++-common/Wmisleading-indentation-level-1.c: New.
	* c-c++-common/Wmisleading-indentation-level-2.c: New.
	* c-c++-common/Wmisleading-indentation.c
	(fn_40_implicit_level_1): New test function.
---
 gcc/c-family/c-indentation.c                       | 82 +++++++++++++++++++---
 gcc/c-family/c.opt                                 |  6 +-
 gcc/doc/invoke.texi                                | 31 +++++++-
 .../c-c++-common/Wmisleading-indentation-in-Wall.c | 30 ++++++++
 .../c-c++-common/Wmisleading-indentation-level-1.c | 35 +++++++++
 .../c-c++-common/Wmisleading-indentation-level-2.c | 34 +++++++++
 .../c-c++-common/Wmisleading-indentation.c         | 37 ++++++++++
 7 files changed, 243 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-in-Wall.c
 create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-level-1.c
 create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-level-2.c

diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index 638a9cf..ca7efdc 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -73,6 +73,42 @@ get_visual_column (expanded_location exploc,
   return true;
 }
 
+/* Determine if the given source line of length LINE_LEN is entirely blank,
+   or contains one or more non-whitespace characters.  */
+
+static bool
+line_is_blank_p (const char *line, int line_len)
+{
+  for (int i = 0; i < line_len; i++)
+    if (!ISSPACE (line[i]))
+      return false;
+
+  return true;
+}
+
+/* Helper function for should_warn_for_misleading_indentation.
+   Determines if there are one or more blank lines between the
+   given source lines.  */
+
+static bool
+separated_by_blank_lines_p (const char *file,
+			    int start_line, int end_line)
+{
+  gcc_assert (start_line < end_line);
+  for (int line_num = start_line; line_num < end_line; line_num++)
+    {
+      int line_len;
+      const char *line = location_get_source_line (file, line_num, &line_len);
+      if (!line)
+	return false;
+
+      if (line_is_blank_p (line, line_len))
+	return true;
+    }
+
+  return false;
+}
+
 /* Does the given source line appear to contain a #if directive?
    (or #ifdef/#ifndef).  Ignore the possibility of it being inside a
    comment, for simplicity.
@@ -166,10 +202,17 @@ detect_preprocessor_logic (expanded_location body_exploc,
    description of that function below.  */
 
 static bool
-should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
+should_warn_for_misleading_indentation (int strictness_level,
+					const token_indent_info &guard_tinfo,
 					const token_indent_info &body_tinfo,
 					const token_indent_info &next_tinfo)
 {
+  /* Early reject for the case where -Wmisleading-indentation is disabled,
+     to avoid doing work only to have the warning suppressed inside the
+     diagnostic machinery.  */
+  if (!strictness_level)
+    return false;
+
   location_t guard_loc = guard_tinfo.location;
   location_t body_loc = body_tinfo.location;
   location_t next_stmt_loc = next_tinfo.location;
@@ -329,6 +372,15 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 	  ;
 	  foo ();
 	  ^ DON'T WARN HERE
+
+     Cases where we may want to issue a warning:
+
+        if (flag)
+           foo ();
+
+           bar ();
+           ^ blank line between guarded and unguarded code
+             Warn here at level 2, but not at level 1.
   */
   if (next_stmt_exploc.line > body_exploc.line)
     {
@@ -354,6 +406,23 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 			      &guard_line_first_nws))
 	return false;
 
+      /* At level 1, don't warn if the guarded and unguarded code are
+	 separated by one or more entirely blank lines, e.g.:
+	   switch (arg)
+	   {
+	   case 0:
+	   if (flagA)
+	     foo (0);
+
+	     break;
+	     ^ DON'T WARN HERE
+	   }
+	 since this is arguably not misleading.  */
+      if (strictness_level < 2)
+	if (separated_by_blank_lines_p (body_exploc.file, body_exploc.line,
+					next_stmt_exploc.line))
+	  return false;
+
       if ((body_type != CPP_SEMICOLON
 	   && next_stmt_vis_column == body_vis_column)
 	  /* As a special case handle the case where the body is a semicolon
@@ -522,17 +591,12 @@ warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 				 const token_indent_info &body_tinfo,
 				 const token_indent_info &next_tinfo)
 {
-  /* Early reject for the case where -Wmisleading-indentation is disabled,
-     to avoid doing work only to have the warning suppressed inside the
-     diagnostic machinery.  */
-  if (!warn_misleading_indentation)
-    return;
-
-  if (should_warn_for_misleading_indentation (guard_tinfo,
+  if (should_warn_for_misleading_indentation (warn_misleading_indentation,
+					      guard_tinfo,
 					      body_tinfo,
 					      next_tinfo))
     {
-      if (warning_at (next_tinfo.location, OPT_Wmisleading_indentation,
+      if (warning_at (next_tinfo.location, OPT_Wmisleading_indentation_,
 		      "statement is indented as if it were guarded by..."))
         inform (guard_tinfo.location,
 		"...this %qs clause, but it is not",
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index aafd802..3cb190d 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -562,7 +562,11 @@ C ObjC C++ ObjC++ Var(warn_memset_transposed_args) Warning LangEnabledBy(C ObjC
 Warn about suspicious calls to memset where the third argument is constant literal zero and the second is not.
 
 Wmisleading-indentation
-C C++ Common Var(warn_misleading_indentation) Warning
+C C++ Common Warning Alias(Wmisleading-indentation=, 1, 0)
+Warn when the indentation of the code does not reflect the block structure.
+
+Wmisleading-indentation=
+C C++ Common Joined RejectNegative UInteger Var(warn_misleading_indentation) Warning LangEnabledBy(C C++,Wall, 1, 0)
 Warn when the indentation of the code does not reflect the block structure.
 
 Wmissing-braces
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 5256031..25a11ef 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -264,7 +264,7 @@ Objective-C and Objective-C++ Dialects}.
 -Winvalid-pch -Wlarger-than=@var{len} @gol
 -Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol
 -Wmain -Wmaybe-uninitialized -Wmemset-transposed-args @gol
--Wmisleading-indentation -Wmissing-braces @gol
+-Wmisleading-indentation -Wmisleading-indentation=@var{n} -Wmissing-braces @gol
 -Wmissing-field-initializers -Wmissing-include-dirs @gol
 -Wno-multichar  -Wnonnull  -Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol
 -Wnull-dereference -Wodr  -Wno-overflow  -Wopenmp-simd  @gol
@@ -3551,6 +3551,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}.
 -Wformat   @gol
 -Wmain @r{(only for C/ObjC and unless} @option{-ffreestanding}@r{)}  @gol
 -Wmaybe-uninitialized @gol
+-Wmisleading-indentation=1 @r{(only for C/C++)} @gol
 -Wmissing-braces @r{(only for C/ObjC)} @gol
 -Wnonnull  @gol
 -Wopenmp-simd @gol
@@ -3889,6 +3890,7 @@ is enabled by default in C++ and is enabled by either @option{-Wall}
 or @option{-Wpedantic}.
 
 @item -Wmisleading-indentation @r{(C and C++ only)}
+@itemx -Wmisleading-indentation=@var{n}
 @opindex Wmisleading-indentation
 @opindex Wno-misleading-indentation
 Warn when the indentation of the code does not reflect the block structure.
@@ -3896,7 +3898,12 @@ Specifically, a warning is issued for @code{if}, @code{else}, @code{while}, and
 @code{for} clauses with a guarded statement that does not use braces,
 followed by an unguarded statement with the same indentation.
 
-This warning is disabled by default.
+This warning is disabled by default.  The warning can optionally accept a
+level (either 1 or 2), for specifying increasing levels of strictness;
+@option{-Wmisleading-indentation} (with no level) is equivalent to
+@option{-Wmisleading-indentation=1}.
+
+@option{-Wmisleading-indentation=1} is enabled by @option{-Wall} in C and C++.
 
 In the following example, the call to ``bar'' is misleadingly indented as
 if it were guarded by the ``if'' conditional.
@@ -3907,6 +3914,26 @@ if it were guarded by the ``if'' conditional.
     bar ();  /* Gotcha: this is not guarded by the "if".  */
 @end smallexample
 
+A warning will be issued for this at @option{-Wmisleading-indentation=1}
+and above.
+
+If there is a blank line between the guarded code and the unguarded code,
+such as in the following:
+
+@smallexample
+  @{
+    foo (0);
+
+  if (some_condition) /* these two lines are not indented enough.  */
+    foo (1);
+
+    foo (2); /* this lines up with "foo (1);" but is not guarded by the "if".  */
+  @}
+@end smallexample
+
+then a warning will be issued at @option{-Wmisleading-indentation=2},
+but not at @option{-Wmisleading-indentation=1}.
+
 In the case of mixed tabs and spaces, the warning uses the
 @option{-ftabstop=} option to determine if the statements line up
 (defaulting to 8).
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-in-Wall.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-in-Wall.c
new file mode 100644
index 0000000..a39e39a
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-in-Wall.c
@@ -0,0 +1,30 @@
+/* { dg-options "-Wall" } */
+/* { dg-do compile } */
+
+/* Verify that -Wmisleading-indentation is enabled by -Wall.  */
+
+int
+fn_1 (int flag)
+{
+  int x = 4, y = 5;
+  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" } */
+    x = 3;
+    y = 2; /* { dg-warning "statement is indented as if it were guarded by..." } */
+  return x * y;
+}
+
+/* Ensure that we can disable the warning.  */
+
+int
+fn_32 (int flag)
+{
+  int x = 4, y = 5;
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wmisleading-indentation"
+  if (flag)
+    x = 3;
+    y = 2;
+#pragma GCC diagnostic pop
+
+  return x * y;
+}
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-1.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-1.c
new file mode 100644
index 0000000..345e324
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-1.c
@@ -0,0 +1,35 @@
+/* { dg-options "-Wmisleading-indentation=1" } */
+/* { dg-do compile } */
+
+extern int foo (int);
+extern int flagA;
+
+/* At -Wmisleading-indentation=1 we shouldn't emit warnings for this
+   function.  Compare with
+   fn_40_implicit_level_1 in Wmisleading-indentation.c and
+   fn_40_level_2 in Wmisleading-indentation-level-2.c.  */
+
+void
+fn_40_level_1 (int arg)
+{
+if (flagA)
+  foo (0);
+
+  foo (1);
+
+
+  if (flagA)
+    foo (0);
+  
+    foo (1);
+
+
+  switch (arg)
+    {
+     case 0:
+     if (flagA)
+       foo (0);
+  
+       break;
+    }
+}
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-2.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-2.c
new file mode 100644
index 0000000..d4c51ea
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-2.c
@@ -0,0 +1,34 @@
+/* { dg-options "-Wmisleading-indentation=2" } */
+/* { dg-do compile } */
+
+extern int foo (int);
+extern int flagA;
+
+/* Compare with fn_40_implicit_level_1 in Wmisleading-indentation.c and
+   fn_40_level_1 in Wmisleading-indentation-level-1.c.
+   Verify that we emit warnings at level 2 for this function.  */
+
+void
+fn_40_level_2 (int arg)
+{
+if (flagA) /* { dg-message "1: ...this 'if' clause" } */
+  foo (0);
+
+  foo (1); /* { dg-warning "statement is indented as if" } */
+
+
+  if (flagA) /* { dg-message "3: ...this 'if' clause" } */
+    foo (0);
+  
+    foo (1); /* { dg-warning "statement is indented as if" } */
+
+
+  switch (arg)
+    {
+     case 0:
+     if (flagA) /* { dg-message "6: ...this 'if' clause" } */
+       foo (0);
+  
+       break; /* { dg-warning "statement is indented as if" } */
+    }
+}
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
index a3f5acd..0e39906 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
@@ -891,3 +891,40 @@ fn_39 (void)
        i++);
   foo (i);
 }
+
+/* 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);
+
+
+  if (flagA)
+    foo (0);
+  
+    foo (1);
+
+
+  switch (arg)
+    {
+     case 0:
+     if (flagA)
+       foo (0);
+  
+       break;
+    }
+}
-- 
1.8.5.3