Improve -Wmisleading-indentation heuristics

gcc/c-family/ChangeLog:

	* c-indentation.c (should_warn_for_misleading_indentation):
	Improve heuristics.

gcc/testsuite/ChangeLog:

	* c-c++-common/Wmisleading-indentation.c: Add more tests.

From-SVN: r226479

gcc/c-family/ChangeLog

 2015-08-02  Patrick Palka  <ppalka@gcc.gnu.org>
 
+	* c-indentation.c (should_warn_for_misleading_indentation):
+	Improve heuristics.
+
+2015-08-02  Patrick Palka  <ppalka@gcc.gnu.org>
+
 	* c-indentation.c (get_visual_column): Add parameter first_nws,
 	use it.  Update comment documenting the function.
 	(is_first_nonwhitespace_on_line): Remove.

gcc/c-family/c-indentation.c

@@ -182,6 +182,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
   location_t body_loc = body_tinfo.location;
   location_t next_stmt_loc = next_tinfo.location;
 
+  enum cpp_ttype body_type = body_tinfo.type;
   enum cpp_ttype next_tok_type = next_tinfo.type;
 
   /* Don't attempt to compare the indentation of BODY_LOC and NEXT_STMT_LOC
@@ -227,12 +228,33 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
       || next_tinfo.keyword == RID_ELSE)
     return false;
 
+  /* Likewise, if the body of the guard is a compound statement then control
+     flow is quite visually explicit regardless of the code's possibly poor
+     indentation, e.g.
+
+     while (foo)  <- GUARD
+       {          <- BODY
+       bar ();
+       }
+       baz ();    <- NEXT
+
+    Things only get muddy when the body of the guard does not have
+    braces, e.g.
+
+    if (foo)  <- GUARD
+      bar (); <- BODY
+      baz (); <- NEXT
+  */
+  if (body_type == CPP_OPEN_BRACE)
+    return false;
+
   /* Don't warn here about spurious semicolons.  */
   if (next_tok_type == CPP_SEMICOLON)
     return false;
 
   expanded_location body_exploc = expand_location (body_loc);
   expanded_location next_stmt_exploc = expand_location (next_stmt_loc);
+  expanded_location guard_exploc = expand_location (guard_loc);
 
   /* They must be in the same file.  */
   if (next_stmt_exploc.file != body_exploc.file)
@@ -250,13 +272,20 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
        if (flag) foo (); bar ();
                          ^ WARN HERE
 
+
+       if (flag) ; {
+                   ^ WARN HERE
+
+       if (flag)
+        ; {
+          ^ WARN HERE
+
      Cases where we don't want to issue a warning:
 
        various_code (); if (flag) foo (); bar (); more_code ();
                                           ^ DON'T WARN HERE.  */
   if (next_stmt_exploc.line == body_exploc.line)
     {
-      expanded_location guard_exploc = expand_location (guard_loc);
       if (guard_exploc.file != body_exploc.file)
 	return true;
       if (guard_exploc.line < body_exploc.line)
@@ -304,14 +333,10 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 	  bar ();
 	  ^ DON'T WARN HERE
 
-        if (flag) {
-          foo ();
-        } else
-        {
-          bar ();
-        }
-        baz ();
-        ^ DON'T WARN HERE
+	if (flag)
+	  ;
+	  foo ();
+	  ^ DON'T WARN HERE
   */
   if (next_stmt_exploc.line > body_exploc.line)
     {
@@ -319,29 +344,84 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 	 "visual column"...  */
       unsigned int next_stmt_vis_column;
       unsigned int body_vis_column;
+      unsigned int body_line_first_nws;
       /* If we can't determine it, don't issue a warning.  This is sometimes
 	 the case for input files containing #line directives, and these
 	 are often for autogenerated sources (e.g. from .md files), where
 	 it's not clear that it's meaningful to look at indentation.  */
       if (!get_visual_column (next_stmt_exploc, &next_stmt_vis_column))
 	return false;
-      if (!get_visual_column (body_exploc, &body_vis_column))
+      if (!get_visual_column (body_exploc,
+			      &body_vis_column,
+			      &body_line_first_nws))
 	return false;
-      if (next_stmt_vis_column == body_vis_column)
+      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
+	     that may be hidden by a preceding comment, e.g.  */
+
+	  // if (p)
+	  //   /* blah */;
+	  //   foo (1);
+
+	  /*  by looking instead at the column of the first non-whitespace
+	      character on the body line.  */
+	  || (body_type == CPP_SEMICOLON
+	      && body_exploc.line > guard_exploc.line
+	      && body_line_first_nws != body_vis_column
+	      && next_stmt_vis_column == body_line_first_nws))
 	{
-	  /* Don't warn if they aren't aligned on the same column
-	     as the guard itself (suggesting autogenerated code that
-	     doesn't bother indenting at all).  */
-	  expanded_location guard_exploc = expand_location (guard_loc);
+          /* Don't warn if they are aligned on the same column
+	     as the guard itself (suggesting autogenerated code that doesn't
+	     bother indenting at all).  We consider the column of the first
+	     non-whitespace character on the guard line instead of the column
+	     of the actual guard token itself because it is more sensible.
+	     Consider:
+
+	     if (p) {
+	     foo (1);
+	     } else     // GUARD
+	     foo (2);   // BODY
+	     foo (3);   // NEXT
+
+	     and:
+
+	     if (p)
+	       foo (1);
+	     } else       // GUARD
+	       foo (2);   // BODY
+	       foo (3);   // NEXT
+
+	     If we just used the column of the guard token, we would warn on
+	     the first example and not warn on the second.  But we want the
+	     exact opposite to happen: to not warn on the first example (which
+	     is probably autogenerated) and to warn on the second (whose
+	     indentation is misleading).  Using the column of the first
+	     non-whitespace character on the guard line makes that
+	     happen.  */
 	  unsigned int guard_vis_column;
-	  if (!get_visual_column (guard_exploc, &guard_vis_column))
+	  unsigned int guard_line_first_nws;
+	  if (!get_visual_column (guard_exploc,
+				  &guard_vis_column,
+				  &guard_line_first_nws))
 	    return false;
-	  if (guard_vis_column == body_vis_column)
+
+	  if (guard_line_first_nws == body_vis_column)
 	    return false;
 
-	  /* PR 66220: Don't warn if the guarding statement is more
-	     indented than the next/body stmts.  */
-	  if (guard_vis_column > body_vis_column)
+	  /* We may have something like:
+
+	     if (p)
+	       {
+	       foo (1);
+	       } else  // GUARD
+	     foo (2);  // BODY
+	     foo (3);  // NEXT
+
+	     in which case the columns are not aligned but the code is not
+	     misleadingly indented.  If the column of the body is less than
+	     that of the guard line then don't warn.  */
+	  if (body_vis_column < guard_line_first_nws)
 	    return false;
 
 	  /* Don't warn if there is multiline preprocessor logic between
@@ -352,6 +432,53 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 	  /* Otherwise, they are visually aligned: issue a warning.  */
 	  return true;
 	}
+
+	/* Also issue a warning for code having the form:
+
+	   if (flag);
+	     foo ();
+
+	   while (flag);
+	   {
+	     ...
+	   }
+
+	   for (...);
+	     {
+	       ...
+	     }
+
+	   if (flag)
+	     ;
+	   else if (flag);
+	     foo ();
+
+	   where the semicolon at the end of each guard is most likely spurious.
+
+	   But do not warn on:
+
+	   for (..);
+	   foo ();
+
+	   where the next statement is aligned with the guard.
+	*/
+	if (body_type == CPP_SEMICOLON)
+	  {
+	    if (body_exploc.line == guard_exploc.line)
+	      {
+		unsigned int guard_vis_column;
+		unsigned int guard_line_first_nws;
+		if (!get_visual_column (guard_exploc,
+					&guard_vis_column,
+					&guard_line_first_nws))
+		  return false;
+
+		if (next_stmt_vis_column > guard_line_first_nws
+		    || (next_tok_type == CPP_OPEN_BRACE
+			&& next_stmt_vis_column == guard_vis_column))
+		  return true;
+	      }
+	  }
     }
 
   return false;

gcc/testsuite/ChangeLog

+2015-08-02  Patrick Palka  <ppalka@gcc.gnu.org>
+
+	* c-c++-common/Wmisleading-indentation.c: Add more tests.
+
 2015-08-01  Michael Collison  <michael.collison@linaro.org
 	    Ramana Radhakrishnan  <ramana.radhakrishnan@linaro.org>
 

gcc/testsuite/c-c++-common/Wmisleading-indentation.c

@@ -691,3 +691,169 @@ fn_36 (void)
 	}
 	foo(6); /* We shouldn't warn here.  */
 }
+
+/* The following function contain code whose indentation is misleading, thus
+   we warn about it.  */
+
+void
+fn_37 (void)
+{
+  int i;
+
+#define EMPTY
+#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++)
+
+  while (flagA); /* { dg-message "3: ...this 'while' clause" } */
+    foo (0); /* { dg-warning "statement is indented as if" } */
+
+  if (flagA)
+    ;
+  else if (flagA); /* { dg-message "8: ...this 'if' clause" } */
+    foo (0); /* { dg-warning "statement is indented as if" } */
+  while (flagA) /* { dg-message "3: ...this 'while' clause" } */
+    /* blah */;
+    foo (0); /* { dg-warning "statement is indented as if" } */
+
+  if (flagA)
+    ;
+  else if (flagA) /* { dg-message "8: ...this 'if' clause" } */
+    foo (1);
+    foo (2); /* { dg-warning "statement is indented as if" } */
+
+  if (flagA)
+    foo (1);
+  else if (flagA) /* { dg-message "8: ...this 'if' clause" } */
+    foo (2);
+    foo (3); /* { dg-warning "statement is indented as if" } */
+
+  if (flagB) /* { dg-message "3: ...this 'if' clause" } */
+    /* blah */;
+    { /* { dg-warning "statement is indented as if" } */
+      foo (0);
+    }
+
+  if (flagB)
+    ;
+  else; foo (0); /* { dg-warning "statement is indented as if" } */
+
+  if (flagC); foo (2); /* { dg-warning "statement is indented as if" } */
+
+  if (flagA)
+    ; /* blah */ { /* { dg-warning "statement is indented as if" } */
+      foo (1);
+    }
+
+  if (flagB) ; /* { dg-message "3: ...this 'if' clause" } */
+    return; /* { dg-warning "statement is indented as if" } */
+
+  if (flagB) EMPTY; /* { dg-message "3: ...this 'if' clause" } */
+    foo (1); /* { dg-warning "statement is indented as if" } */
+
+  for (i = 0; i < 10; i++); /* { dg-message "3: ...this 'for' clause" } */
+    foo (2); /* { dg-warning "statement is indented as if" } */
+
+  FOR_EACH (i, 0, 10);
+    foo (2); /* { dg-warning "statement is indented as if" } */
+
+  FOR_EACH (i, 0, 10);
+    { /* { dg-warning "statement is indented as if" } */
+      foo (3);
+    }
+
+  FOR_EACH (i, 0, 10);
+  { /* { dg-warning "statement is indented as if" } */
+    foo (3);
+  }
+
+  while (i++); { /* { dg-warning "statement is indented as if" } */
+    foo (3);
+  }
+
+  if (i++); { /* { dg-warning "statement is indented as if" } */
+    foo (3);
+  }
+
+  if (flagA) {
+    foo (1);
+  } else /* { dg-message "5: ...this 'else' clause" } */
+    if (flagB)
+       foo (2);
+    foo (3); /* { dg-warning "statement is indented as if" } */
+
+  if (flagA)
+    foo (1);
+  else if (flagB); /* { dg-message "8: ...this 'if' clause" } */
+    foo (2); /* { dg-warning "statement is indented as if" } */
+
+#undef EMPTY
+#undef FOR_EACH
+}
+
+/* The following function contains code whose indentation is not great but not
+   misleading, thus we don't warn.  */
+
+void
+fn_38 (void)
+{
+  int i = 0;
+
+  while (flagA)
+    ;
+    foo (0);
+
+  if (flagB)
+    ;
+    {
+      foo (0);
+    }
+
+  while (flagC);
+  foo (2);
+
+  if (flagA)
+    while (flagC++);
+  else
+    foo (2);
+
+  if (i)
+    while (i++ < 10000);
+  foo (5);
+
+  if (i) while (i++ < 10000);
+  foo (5);
+
+  if (flagA) {
+    foo (1);
+  } else
+  if (flagB)
+    foo (2);
+  foo (3);
+
+  if (flagA)
+    {
+    foo (1);
+    } else
+  if (flagB)
+    foo (2);
+  foo (3);
+}
+
+/* The following function contains good indentation which we definitely should
+   not warn about.  */
+
+void
+fn_39 (void)
+{
+  if (flagA)
+    ;
+  if (flagB)
+    ;
+
+  if (flagA)
+    if (flagB)
+      foo (0);
+    else
+      foo (1);
+  else
+    foo (2);
+}