[PATCH 2/3] Remove is_first_nonwhitespace_on_line(), instead improve get_visual_column()

[PATCH 2/3] Remove is_first_nonwhitespace_on_line(), instead improve get_visual_column()

[Patrick Palka]

This patch removes the function is_first_nonwhitespace_on_line() in
favor of augmenting the function get_visual_column() to optionally
return the visual column corresponding to the first non-whitespace character
on the line.  Existing usage of is_first_nonwhitespace_on_line() can
be trivially replaced by calling get_visual_column() and comparing *out
with *first_nws.

The rationale for this change is that in many cases it is better to use
the visual column of the first non-whitespace character rather than the
visual column of the token.  Consider:

  if (p) {
    foo (1);
  } else       // GUARD
    if (q)     // BODY
      foo (2);
    foo (3);   // NEXT

Here, with current heuristics, we do not emit a warning because we
notice that the visual columns of each token line up ("suggesting"
autogenerated code).  Yet it is obvious that we should warn here because
it misleadingly looks like the foo (3); statement is guarded by the
else.

If we instead consider the visual column of the first non-whitespace
character on the guard line, the columns will not line up thus we will
emit the warning.  This will be done in the next patch.

gcc/c-family/ChangeLog:

	* c-indentation.c (get_visual_column): Add parameter first_nws,
	use it.  Update comment documenting the function.
	(is_first_nonwhitespace_on_line): Remove.
	(should_warn_for_misleading_indentation): Replace usage of
	of is_first_nonwhitespace_on_line with get_visual_column.
---
 gcc/c-family/c-indentation.c | 53 ++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index 9043f8c..f43aee6 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -36,11 +36,16 @@ extern cpp_options *cpp_opts;
 /* Convert libcpp's notion of a column (a 1-based char count) to
    the "visual column" (0-based column, respecting tabs), by reading the
    relevant line.
+
    Returns true if a conversion was possible, writing the result to OUT,
-   otherwise returns false.  */
+   otherwise returns false.  If FIRST_NWS is not NULL, then write to it
+   the visual column corresponding to the first non-whitespace character
+   on the line.  */
 
 static bool
-get_visual_column (expanded_location exploc, unsigned int *out)
+get_visual_column (expanded_location exploc,
+		   unsigned int *out,
+		   unsigned int *first_nws = NULL)
 {
   int line_len;
   const char *line = location_get_source_line (exploc, &line_len);
@@ -50,6 +55,13 @@ get_visual_column (expanded_location exploc, unsigned int *out)
   for (int i = 1; i < exploc.column; i++)
     {
       unsigned char ch = line[i - 1];
+
+      if (first_nws != NULL && !ISSPACE (ch))
+	{
+	  *first_nws = vis_column;
+	  first_nws = NULL;
+	}
+
       if (ch == '\t')
        {
 	 /* Round up to nearest tab stop. */
@@ -60,36 +72,13 @@ get_visual_column (expanded_location exploc, unsigned int *out)
        vis_column++;
     }
 
+  if (first_nws != NULL)
+    *first_nws = vis_column;
+
   *out = vis_column;
   return true;
 }
 
-/* Is the token at LOC the first non-whitespace on its line?
-   Helper function for should_warn_for_misleading_indentation.  */
-
-static bool
-is_first_nonwhitespace_on_line (expanded_location exploc)
-{
-  int line_len;
-  const char *line = location_get_source_line (exploc, &line_len);
-
-   /* If we can't determine it, return false so that we 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 (!line)
-    return false;
-
-  for (int i = 1; i < exploc.column; i++)
-    {
-      unsigned char ch = line[i - 1];
-      if (!ISSPACE (ch))
-	return false;
-    }
-  return true;
-}
-
 /* 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.
@@ -282,9 +271,15 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 	  /* They're all on the same line.  */
 	  gcc_assert (guard_exploc.file == next_stmt_exploc.file);
 	  gcc_assert (guard_exploc.line == next_stmt_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;
 	  /* Heuristic: only warn if the guard is the first thing
 	     on its line.  */
-	  if (is_first_nonwhitespace_on_line (guard_exploc))
+	  if (guard_vis_column == guard_line_first_nws)
 	    return true;
 	}
     }
-- 
2.4.3.368.g7974889.dirty

[PATCH 1/3] Refactor entry point to -Wmisleading-indentation

[Patrick Palka]

This patch refactors the entry point of -Wmisleading-indentation from:

  void
  warn_for_misleading_indentation (location_t guard_loc,
                                   location_t body_loc,
                                   location_t next_stmt_loc,
                                   enum cpp_ttype next_tok_type,
                                   const char *guard_kind);

to

  struct token_indent_info
  {
    location_t location;
    cpp_ttype type;
    rid keyword;
  };

  void
  warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
                                   const token_indent_info &body_tinfo,
                                   const token_indent_info &next_tinfo);

The purpose of this refactoring is to expose more information to the
-Wmisleading-indentation implementation to allow for more advanced
heuristics and for better coverage.

(I decided to keep the usage of const references because nobody
seems to mind.  Also I added a new header file, c-indentation.h.)

gcc/c-family/ChangeLog:

	* c-indentation.h (struct token_indent_info): Define.
	(get_token_indent_info): Define.
	(warn_for_misleading_information): Declare.
	* c-common.h (warn_for_misleading_information): Remove.
	* c-identation.c (warn_for_misleading_indentation):
	Change declaration to take three token_indent_infos.  Adjust
	accordingly.
	* c-identation.c (should_warn_for_misleading_indentation):
	Likewise.  Bail out early if the body is a compound statement.
	(guard_tinfo_to_string): Define.

gcc/c/ChangeLog:

	* c-parser.c (c_parser_if_body): Take token_indent_info
	argument. Call warn_for_misleading_indentation even when the
	body is a semicolon.  Extract token_indent_infos corresponding
	to the guard, body and next tokens.  Adjust call to
	warn_for_misleading_indentation accordingly.
	(c_parser_else_body): Likewise.
	(c_parser_if_statement): Likewise.
	(c_parser_while_statement): Likewise.
	(c_parser_for_statement): Likewise.

gcc/cp/ChangeLog:

	* parser.c (cp_parser_selection_statement): Move handling of
	semicolon body to ...
	(cp_parser_implicitly_scoped_statement): .. here.  Call
	warn_for_misleading_indentation even when the body is a
	semicolon.  Extract token_indent_infos corresponding to the
	guard, body and next tokens.  Adjust call to
	warn_for_misleading_indentation accordingly.  Take
	token_indent_info argument.
	(cp_parser_already_scoped_statement): Likewise.
	(cp_parser_selection_statement, cp_parser_iteration_statement):
	Extract a token_indent_info corresponding to the guard token.
---
 gcc/c-family/c-common.h      |   7 ---
 gcc/c-family/c-indentation.c |  79 ++++++++++++++++++++++++++--------
 gcc/c-family/c-indentation.h |  52 ++++++++++++++++++++++
 gcc/c/c-parser.c             |  81 +++++++++++++++++++----------------
 gcc/cp/parser.c              | 100 +++++++++++++++++++------------------------
 5 files changed, 201 insertions(+), 118 deletions(-)
 create mode 100644 gcc/c-family/c-indentation.h

diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 7ba81c4..a78a13c 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1429,12 +1429,5 @@ extern bool contains_cilk_spawn_stmt (tree);
 extern tree cilk_for_number_of_iterations (tree);
 extern bool check_no_cilk (tree, const char *, const char *,
 		           location_t loc = UNKNOWN_LOCATION);
-/* In c-indentation.c.  */
-extern void
-warn_for_misleading_indentation (location_t guard_loc,
-				 location_t body_loc,
-				 location_t next_stmt_loc,
-				 enum cpp_ttype next_tok_type,
-				 const char *guard_kind);
 
 #endif /* ! GCC_C_COMMON_H */
diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index f8c28cf..9043f8c 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -29,6 +29,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "stor-layout.h"
 #include "input.h"
 #include "c-common.h"
+#include "c-indentation.h"
 
 extern cpp_options *cpp_opts;
 
@@ -187,11 +188,16 @@ detect_preprocessor_logic (expanded_location body_exploc,
    description of that function below.  */
 
 static bool
-should_warn_for_misleading_indentation (location_t guard_loc,
-					location_t body_loc,
-					location_t next_stmt_loc,
-					enum cpp_ttype next_tok_type)
+should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
+					const token_indent_info &body_tinfo,
+					const token_indent_info &next_tinfo)
 {
+  location_t guard_loc = guard_tinfo.location;
+  location_t body_loc = body_tinfo.location;
+  location_t next_stmt_loc = next_tinfo.location;
+
+  enum cpp_ttype next_tok_type = next_tinfo.type;
+
   /* Don't attempt to compare the indentation of BODY_LOC and NEXT_STMT_LOC
      if either are within macros.  */
   if (linemap_location_from_macro_expansion_p (line_table, body_loc)
@@ -217,7 +223,22 @@ should_warn_for_misleading_indentation (location_t guard_loc,
   if (line_table->seen_line_directive)
     return false;
 
-  if (next_tok_type == CPP_CLOSE_BRACE)
+  /* If the token following the body is a close brace or an "else"
+     then while indentation may be sloppy, there is not much ambiguity
+     about control flow, e.g.
+
+     if (foo)       <- GUARD
+       bar ();      <- BODY
+       else baz (); <- NEXT
+
+     {
+     while (foo)  <- GUARD
+     bar ();      <- BODY
+     }            <- NEXT
+     baz ();
+  */
+  if (next_tok_type == CPP_CLOSE_BRACE
+      || next_tinfo.keyword == RID_ELSE)
     return false;
 
   /* Don't warn here about spurious semicolons.  */
@@ -344,6 +365,28 @@ should_warn_for_misleading_indentation (location_t guard_loc,
   return false;
 }
 
+/* Return the string identifier corresponding to the given guard token.  */
+
+static const char *
+guard_tinfo_to_string (const token_indent_info &guard_tinfo)
+{
+  switch (guard_tinfo.keyword)
+    {
+    case RID_FOR:
+      return "for";
+    case RID_ELSE:
+      return "else";
+    case RID_IF:
+      return "if";
+    case RID_WHILE:
+      return "while";
+    case RID_DO:
+      return "do";
+    default:
+      gcc_unreachable ();
+    }
+}
+
 /* Called by the C/C++ frontends when we have a guarding statement at
    GUARD_LOC containing a statement at BODY_LOC, where the block wasn't
    written using braces, like this:
@@ -371,11 +414,9 @@ should_warn_for_misleading_indentation (location_t guard_loc,
    GUARD_KIND identifies the kind of clause e.g. "if", "else" etc.  */
 
 void
-warn_for_misleading_indentation (location_t guard_loc,
-				 location_t body_loc,
-				 location_t next_stmt_loc,
-				 enum cpp_ttype next_tok_type,
-				 const char *guard_kind)
+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
@@ -383,12 +424,14 @@ warn_for_misleading_indentation (location_t guard_loc,
   if (!warn_misleading_indentation)
     return;
 
-  if (should_warn_for_misleading_indentation (guard_loc,
-					      body_loc,
-					      next_stmt_loc,
-					      next_tok_type))
-    if (warning_at (next_stmt_loc, OPT_Wmisleading_indentation,
-		    "statement is indented as if it were guarded by..."))
-      inform (guard_loc,
-	      "...this %qs clause, but it is not", guard_kind);
+  if (should_warn_for_misleading_indentation (guard_tinfo,
+					      body_tinfo,
+					      next_tinfo))
+    {
+      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",
+		guard_tinfo_to_string (guard_tinfo));
+    }
 }
diff --git a/gcc/c-family/c-indentation.h b/gcc/c-family/c-indentation.h
new file mode 100644
index 0000000..7fb6bb4
--- /dev/null
+++ b/gcc/c-family/c-indentation.h
@@ -0,0 +1,52 @@
+/* Definitions for c-indentation.c.
+   Copyright (C) 2015 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+#ifndef GCC_C_INDENTATION_H
+#define GCC_C_INDENTATION_H
+
+/* Token information used by the -Wmisleading-indentation implementation.  */
+
+struct token_indent_info
+{
+  location_t location;
+  ENUM_BITFIELD (cpp_ttype) type : 8;
+  ENUM_BITFIELD (rid) keyword : 8;
+};
+
+/* Extract token information from TOKEN, which ought to either be a
+   cp_token * or a c_token *.  */
+
+template <typename T>
+inline token_indent_info
+get_token_indent_info (const T *token)
+{
+  token_indent_info info;
+  info.location = token->location;
+  info.type = token->type;
+  info.keyword = token->keyword;
+
+  return info;
+}
+
+extern void
+warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
+				 const token_indent_info &body_tinfo,
+				 const token_indent_info &next_tinfo);
+
+#endif  /* ! GCC_C_INDENTATION_H  */
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 0f6d1c9..85a6779 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -71,6 +71,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "omp-low.h"
 #include "builtins.h"
 #include "gomp-constants.h"
+#include "c-family/c-indentation.h"
 
 \f
 /* Initialization routine for this file.  */
@@ -5185,10 +5186,14 @@ c_parser_c99_block_statement (c_parser *parser)
    parser->in_if_block.  */
 
 static tree
-c_parser_if_body (c_parser *parser, bool *if_p, location_t if_loc)
+c_parser_if_body (c_parser *parser, bool *if_p,
+		  const token_indent_info &if_tinfo)
 {
   tree block = c_begin_compound_stmt (flag_isoc99);
   location_t body_loc = c_parser_peek_token (parser)->location;
+  token_indent_info body_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
+
   c_parser_all_labels (parser);
   *if_p = c_parser_next_token_is_keyword (parser, RID_IF);
   if (c_parser_next_token_is (parser, CPP_SEMICOLON))
@@ -5203,14 +5208,11 @@ c_parser_if_body (c_parser *parser, bool *if_p, location_t if_loc)
   else if (c_parser_next_token_is (parser, CPP_OPEN_BRACE))
     add_stmt (c_parser_compound_statement (parser));
   else
-    {
-      c_parser_statement_after_labels (parser);
-      if (!c_parser_next_token_is_keyword (parser, RID_ELSE))
-	warn_for_misleading_indentation (if_loc, body_loc,
-					 c_parser_peek_token (parser)->location,
-					 c_parser_peek_token (parser)->type,
-					 "if");
-    }
+    c_parser_statement_after_labels (parser);
+
+  token_indent_info next_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
+  warn_for_misleading_indentation (if_tinfo, body_tinfo, next_tinfo);
 
   return c_end_compound_stmt (body_loc, block, flag_isoc99);
 }
@@ -5220,10 +5222,13 @@ c_parser_if_body (c_parser *parser, bool *if_p, location_t if_loc)
    specially for the sake of -Wempty-body warnings.  */
 
 static tree
-c_parser_else_body (c_parser *parser, location_t else_loc)
+c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo)
 {
   location_t body_loc = c_parser_peek_token (parser)->location;
   tree block = c_begin_compound_stmt (flag_isoc99);
+  token_indent_info body_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
+
   c_parser_all_labels (parser);
   if (c_parser_next_token_is (parser, CPP_SEMICOLON))
     {
@@ -5235,13 +5240,12 @@ c_parser_else_body (c_parser *parser, location_t else_loc)
       c_parser_consume_token (parser);
     }
   else
-    {
-      c_parser_statement_after_labels (parser);
-      warn_for_misleading_indentation (else_loc, body_loc,
-				       c_parser_peek_token (parser)->location,
-				       c_parser_peek_token (parser)->type,
-				       "else");
-    }
+    c_parser_statement_after_labels (parser);
+
+  token_indent_info next_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
+  warn_for_misleading_indentation (else_tinfo, body_tinfo, next_tinfo);
+
   return c_end_compound_stmt (body_loc, block, flag_isoc99);
 }
 
@@ -5264,7 +5268,8 @@ c_parser_if_statement (c_parser *parser)
   tree if_stmt;
 
   gcc_assert (c_parser_next_token_is_keyword (parser, RID_IF));
-  location_t if_loc = c_parser_peek_token (parser)->location;
+  token_indent_info if_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
   c_parser_consume_token (parser);
   block = c_begin_compound_stmt (flag_isoc99);
   loc = c_parser_peek_token (parser)->location;
@@ -5276,13 +5281,14 @@ c_parser_if_statement (c_parser *parser)
     }
   in_if_block = parser->in_if_block;
   parser->in_if_block = true;
-  first_body = c_parser_if_body (parser, &first_if, if_loc);
+  first_body = c_parser_if_body (parser, &first_if, if_tinfo);
   parser->in_if_block = in_if_block;
   if (c_parser_next_token_is_keyword (parser, RID_ELSE))
     {
-      location_t else_loc = c_parser_peek_token (parser)->location;
+      token_indent_info else_tinfo
+	= get_token_indent_info (c_parser_peek_token (parser));
       c_parser_consume_token (parser);
-      second_body = c_parser_else_body (parser, else_loc);
+      second_body = c_parser_else_body (parser, else_tinfo);
     }
   else
     second_body = NULL_TREE;
@@ -5362,7 +5368,8 @@ c_parser_while_statement (c_parser *parser, bool ivdep)
   tree block, cond, body, save_break, save_cont;
   location_t loc;
   gcc_assert (c_parser_next_token_is_keyword (parser, RID_WHILE));
-  location_t while_loc = c_parser_peek_token (parser)->location;
+  token_indent_info while_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
   c_parser_consume_token (parser);
   block = c_begin_compound_stmt (flag_isoc99);
   loc = c_parser_peek_token (parser)->location;
@@ -5380,14 +5387,14 @@ c_parser_while_statement (c_parser *parser, bool ivdep)
   save_cont = c_cont_label;
   c_cont_label = NULL_TREE;
 
-  location_t body_loc = UNKNOWN_LOCATION;
-  if (c_parser_peek_token (parser)->type != CPP_OPEN_BRACE)
-    body_loc = c_parser_peek_token (parser)->location;
+  token_indent_info body_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
+
   body = c_parser_c99_block_statement (parser);
-  warn_for_misleading_indentation (while_loc, body_loc,
-				   c_parser_peek_token (parser)->location,
-				   c_parser_peek_token (parser)->type,
-				   "while");
+
+  token_indent_info next_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
+  warn_for_misleading_indentation (while_tinfo, body_tinfo, next_tinfo);
 
   c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label, true);
   add_stmt (c_end_compound_stmt (loc, block, flag_isoc99));
@@ -5507,6 +5514,8 @@ c_parser_for_statement (c_parser *parser, bool ivdep)
   location_t for_loc = c_parser_peek_token (parser)->location;
   bool is_foreach_statement = false;
   gcc_assert (c_parser_next_token_is_keyword (parser, RID_FOR));
+  token_indent_info for_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
   c_parser_consume_token (parser);
   /* Open a compound statement in Objective-C as well, just in case this is
      as foreach expression.  */
@@ -5667,14 +5676,14 @@ c_parser_for_statement (c_parser *parser, bool ivdep)
   save_cont = c_cont_label;
   c_cont_label = NULL_TREE;
 
-  location_t body_loc = UNKNOWN_LOCATION;
-  if (c_parser_peek_token (parser)->type != CPP_OPEN_BRACE)
-    body_loc = c_parser_peek_token (parser)->location;
+  token_indent_info body_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
+
   body = c_parser_c99_block_statement (parser);
-  warn_for_misleading_indentation (for_loc, body_loc,
-				   c_parser_peek_token (parser)->location,
-				   c_parser_peek_token (parser)->type,
-				   "for");
+
+  token_indent_info next_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
+  warn_for_misleading_indentation (for_tinfo, body_tinfo, next_tinfo);
 
   if (is_foreach_statement)
     objc_finish_foreach_loop (loc, object_expression, collection_expression, body, c_break_label, c_cont_label);
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 15b920a..5fdb2a7 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -54,6 +54,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "type-utils.h"
 #include "omp-low.h"
 #include "gomp-constants.h"
+#include "c-family/c-indentation.h"
 
 \f
 /* The lexer.  */
@@ -2058,9 +2059,9 @@ static void cp_parser_declaration_statement
   (cp_parser *);
 
 static tree cp_parser_implicitly_scoped_statement
-  (cp_parser *, bool *, location_t, const char *);
+  (cp_parser *, bool *, const token_indent_info &);
 static void cp_parser_already_scoped_statement
-  (cp_parser *, location_t, const char *);
+  (cp_parser *, const token_indent_info &);
 
 /* Declarations [gram.dcl.dcl] */
 
@@ -10108,12 +10109,14 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p)
 {
   cp_token *token;
   enum rid keyword;
+  token_indent_info guard_tinfo;
 
   if (if_p != NULL)
     *if_p = false;
 
   /* Peek at the next token.  */
   token = cp_parser_require (parser, CPP_KEYWORD, RT_SELECT);
+  guard_tinfo = get_token_indent_info (token);
 
   /* See what kind of keyword it is.  */
   keyword = token->keyword;
@@ -10156,19 +10159,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p)
 	    /* Parse the then-clause.  */
 	    in_statement = parser->in_statement;
 	    parser->in_statement |= IN_IF_STMT;
-	    if (cp_lexer_next_token_is (parser->lexer, CPP_SEMICOLON))
-	      {
-	        location_t loc = cp_lexer_peek_token (parser->lexer)->location;
-		add_stmt (build_empty_stmt (loc));
-		cp_lexer_consume_token (parser->lexer);
-	        if (!cp_lexer_next_token_is_keyword (parser->lexer, RID_ELSE))
-		  warning_at (loc, OPT_Wempty_body, "suggest braces around "
-			      "empty body in an %<if%> statement");
-		nested_if = false;
-	      }
-	    else
-	      cp_parser_implicitly_scoped_statement (parser, &nested_if,
-						     token->location, "if");
+	    cp_parser_implicitly_scoped_statement (parser, &nested_if,
+						   guard_tinfo);
 	    parser->in_statement = in_statement;
 
 	    finish_then_clause (statement);
@@ -10177,24 +10169,14 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p)
 	    if (cp_lexer_next_token_is_keyword (parser->lexer,
 						RID_ELSE))
 	      {
+		guard_tinfo
+		  = get_token_indent_info (cp_lexer_peek_token (parser->lexer));
 		/* Consume the `else' keyword.  */
-		location_t else_tok_loc
-		  = cp_lexer_consume_token (parser->lexer)->location;
+		cp_lexer_consume_token (parser->lexer);
 		begin_else_clause (statement);
 		/* Parse the else-clause.  */
-	        if (cp_lexer_next_token_is (parser->lexer, CPP_SEMICOLON))
-	          {
-		    location_t loc;
-		    loc = cp_lexer_peek_token (parser->lexer)->location;
-		    warning_at (loc,
-				OPT_Wempty_body, "suggest braces around "
-			        "empty body in an %<else%> statement");
-		    add_stmt (build_empty_stmt (loc));
-		    cp_lexer_consume_token (parser->lexer);
-		  }
-		else
-		  cp_parser_implicitly_scoped_statement (parser, NULL,
-							 else_tok_loc, "else");
+		cp_parser_implicitly_scoped_statement (parser, NULL,
+						       guard_tinfo);
 
 		finish_else_clause (statement);
 
@@ -10235,7 +10217,7 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p)
 	    parser->in_switch_statement_p = true;
 	    parser->in_statement |= IN_SWITCH_STMT;
 	    cp_parser_implicitly_scoped_statement (parser, NULL,
-						   0, "switch");
+						   guard_tinfo);
 	    parser->in_switch_statement_p = in_switch_statement_p;
 	    parser->in_statement = in_statement;
 
@@ -10780,17 +10762,17 @@ static tree
 cp_parser_iteration_statement (cp_parser* parser, bool ivdep)
 {
   cp_token *token;
-  location_t tok_loc;
   enum rid keyword;
   tree statement;
   unsigned char in_statement;
+  token_indent_info guard_tinfo;
 
   /* Peek at the next token.  */
   token = cp_parser_require (parser, CPP_KEYWORD, RT_INTERATION);
   if (!token)
     return error_mark_node;
 
-  tok_loc = token->location;
+  guard_tinfo = get_token_indent_info (token);
 
   /* Remember whether or not we are already within an iteration
      statement.  */
@@ -10815,7 +10797,7 @@ cp_parser_iteration_statement (cp_parser* parser, bool ivdep)
 	cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
 	/* Parse the dependent statement.  */
 	parser->in_statement = IN_ITERATION_STMT;
-	cp_parser_already_scoped_statement (parser, tok_loc, "while");
+	cp_parser_already_scoped_statement (parser, guard_tinfo);
 	parser->in_statement = in_statement;
 	/* We're done with the while-statement.  */
 	finish_while_stmt (statement);
@@ -10830,7 +10812,7 @@ cp_parser_iteration_statement (cp_parser* parser, bool ivdep)
 	statement = begin_do_stmt ();
 	/* Parse the body of the do-statement.  */
 	parser->in_statement = IN_ITERATION_STMT;
-	cp_parser_implicitly_scoped_statement (parser, NULL, 0, "do");
+	cp_parser_implicitly_scoped_statement (parser, NULL, guard_tinfo);
 	parser->in_statement = in_statement;
 	finish_do_body (statement);
 	/* Look for the `while' keyword.  */
@@ -10860,7 +10842,7 @@ cp_parser_iteration_statement (cp_parser* parser, bool ivdep)
 
 	/* Parse the body of the for-statement.  */
 	parser->in_statement = IN_ITERATION_STMT;
-	cp_parser_already_scoped_statement (parser, tok_loc, "for");
+	cp_parser_already_scoped_statement (parser, guard_tinfo);
 	parser->in_statement = in_statement;
 
 	/* We're done with the for-statement.  */
@@ -11130,10 +11112,12 @@ cp_parser_declaration_statement (cp_parser* parser)
 
 static tree
 cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
-				       location_t guard_loc,
-				       const char *guard_kind)
+				       const token_indent_info &guard_tinfo)
 {
   tree statement;
+  location_t body_loc = cp_lexer_peek_token (parser->lexer)->location;
+  token_indent_info body_tinfo
+    = get_token_indent_info (cp_lexer_peek_token (parser->lexer));
 
   if (if_p != NULL)
     *if_p = false;
@@ -11141,9 +11125,16 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
   /* Mark if () ; with a special NOP_EXPR.  */
   if (cp_lexer_next_token_is (parser->lexer, CPP_SEMICOLON))
     {
-      location_t loc = cp_lexer_peek_token (parser->lexer)->location;
       cp_lexer_consume_token (parser->lexer);
-      statement = add_stmt (build_empty_stmt (loc));
+      statement = add_stmt (build_empty_stmt (body_loc));
+
+      if (guard_tinfo.keyword == RID_IF
+	  && !cp_lexer_next_token_is_keyword (parser->lexer, RID_ELSE))
+	warning_at (body_loc, OPT_Wempty_body,
+		    "suggest braces around empty body in an %<if%> statement");
+      else if (guard_tinfo.keyword == RID_ELSE)
+	warning_at (body_loc, OPT_Wempty_body,
+		    "suggest braces around empty body in an %<else%> statement");
     }
   /* if a compound is opened, we simply parse the statement directly.  */
   else if (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE))
@@ -11154,20 +11145,15 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
       /* Create a compound-statement.  */
       statement = begin_compound_stmt (0);
       /* Parse the dependent-statement.  */
-      location_t body_loc = cp_lexer_peek_token (parser->lexer)->location;
       cp_parser_statement (parser, NULL_TREE, false, if_p);
       /* Finish the dummy compound-statement.  */
       finish_compound_stmt (statement);
-      cp_token *next_tok = cp_lexer_peek_token (parser->lexer);
-      if (next_tok->keyword != RID_ELSE)
-        {
-          location_t next_stmt_loc = next_tok->location;
-          warn_for_misleading_indentation (guard_loc, body_loc,
-                                           next_stmt_loc, next_tok->type,
-                                           guard_kind);
-        }
     }
 
+  token_indent_info next_tinfo
+    = get_token_indent_info (cp_lexer_peek_token (parser->lexer));
+  warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo);
+
   /* Return the statement.  */
   return statement;
 }
@@ -11178,19 +11164,19 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
    scope.  */
 
 static void
-cp_parser_already_scoped_statement (cp_parser* parser, location_t guard_loc,
-				    const char *guard_kind)
+cp_parser_already_scoped_statement (cp_parser* parser,
+				    const token_indent_info &guard_tinfo)
 {
   /* If the token is a `{', then we must take special action.  */
   if (cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE))
     {
-      location_t body_loc = cp_lexer_peek_token (parser->lexer)->location;
+      token_indent_info body_tinfo
+	= get_token_indent_info (cp_lexer_peek_token (parser->lexer));
+
       cp_parser_statement (parser, NULL_TREE, false, NULL);
-      cp_token *next_tok = cp_lexer_peek_token (parser->lexer);
-      location_t next_stmt_loc = next_tok->location;
-      warn_for_misleading_indentation (guard_loc, body_loc,
-                                       next_stmt_loc, next_tok->type,
-                                       guard_kind);
+      token_indent_info next_tinfo
+	= get_token_indent_info (cp_lexer_peek_token (parser->lexer));
+      warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo);
     }
   else
     {
-- 
2.4.3.368.g7974889.dirty

[PATCH 3/3] Improve -Wmissing-indentation heuristics

[Patrick Palka]

This patch improves the heuristics of the warning in a number of ways.
The improvements are hopefully adequately documented in the code
comments.

The additions to the test case also highlight the improvements.

I tested an earlier version of this patch on more than a dozen C code
bases.  I only found one class of bogus warnings yet emitted, in the
libpng and bdwgc projects.  These projects have a coding style which
indents code inside #ifdefs as if this code was guarded by an if(), e.g.

  if (foo != 0)
    x = 10;
  else       // GUARD
    y = 100; // BODY

  #ifdef BAR
    blah ();  // NEXT
  #endif

These bogus warnings are pre-existing, however (i.e. not caused by this
patch).

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.
---
 gcc/c-family/c-indentation.c                       | 167 ++++++++++++++++++---
 .../c-c++-common/Wmisleading-indentation.c         | 166 ++++++++++++++++++++
 2 files changed, 313 insertions(+), 20 deletions(-)

diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index f43aee6..8de3bc5 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -185,6 +185,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
@@ -230,12 +231,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)
@@ -253,13 +275,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)
@@ -307,14 +336,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)
     {
@@ -322,29 +347,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
@@ -355,6 +435,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;
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
index 443e3dd..0d6d8d2 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
+++ b/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);
+}
-- 
2.4.3.368.g7974889.dirty

Re: [PATCH 1/3] Refactor entry point to -Wmisleading-indentation

[Patrick Palka]

On Tue, Jun 9, 2015 at 1:31 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> This patch refactors the entry point of -Wmisleading-indentation from:
>
>   void
>   warn_for_misleading_indentation (location_t guard_loc,
>                                    location_t body_loc,
>                                    location_t next_stmt_loc,
>                                    enum cpp_ttype next_tok_type,
>                                    const char *guard_kind);
>
> to
>
>   struct token_indent_info
>   {
>     location_t location;
>     cpp_ttype type;
>     rid keyword;
>   };
>
>   void
>   warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
>                                    const token_indent_info &body_tinfo,
>                                    const token_indent_info &next_tinfo);
>
> The purpose of this refactoring is to expose more information to the
> -Wmisleading-indentation implementation to allow for more advanced
> heuristics and for better coverage.
>
> (I decided to keep the usage of const references because nobody
> seems to mind.  Also I added a new header file, c-indentation.h.)
>
> gcc/c-family/ChangeLog:
>
>         * c-indentation.h (struct token_indent_info): Define.
>         (get_token_indent_info): Define.
>         (warn_for_misleading_information): Declare.
>         * c-common.h (warn_for_misleading_information): Remove.
>         * c-identation.c (warn_for_misleading_indentation):
>         Change declaration to take three token_indent_infos.  Adjust
>         accordingly.
>         * c-identation.c (should_warn_for_misleading_indentation):
>         Likewise.  Bail out early if the body is a compound statement.
>         (guard_tinfo_to_string): Define.
>
> gcc/c/ChangeLog:
>
>         * c-parser.c (c_parser_if_body): Take token_indent_info
>         argument. Call warn_for_misleading_indentation even when the
>         body is a semicolon.  Extract token_indent_infos corresponding
>         to the guard, body and next tokens.  Adjust call to
>         warn_for_misleading_indentation accordingly.
>         (c_parser_else_body): Likewise.
>         (c_parser_if_statement): Likewise.
>         (c_parser_while_statement): Likewise.
>         (c_parser_for_statement): Likewise.
>
> gcc/cp/ChangeLog:
>
>         * parser.c (cp_parser_selection_statement): Move handling of
>         semicolon body to ...
>         (cp_parser_implicitly_scoped_statement): .. here.  Call
>         warn_for_misleading_indentation even when the body is a
>         semicolon.  Extract token_indent_infos corresponding to the
>         guard, body and next tokens.  Adjust call to
>         warn_for_misleading_indentation accordingly.  Take
>         token_indent_info argument.
>         (cp_parser_already_scoped_statement): Likewise.
>         (cp_parser_selection_statement, cp_parser_iteration_statement):
>         Extract a token_indent_info corresponding to the guard token.

Pinging this series.

Re: [PATCH 3/3] Improve -Wmissing-indentation heuristics

[David Malcolm]

On Tue, 2015-06-09 at 13:31 -0400, Patrick Palka wrote:
> This patch improves the heuristics of the warning in a number of ways.
> The improvements are hopefully adequately documented in the code
> comments.
> 
> The additions to the test case also highlight the improvements.
> 
> I tested an earlier version of this patch on more than a dozen C code
> bases.  I only found one class of bogus warnings yet emitted, in the
> libpng and bdwgc projects.  These projects have a coding style which
> indents code inside #ifdefs as if this code was guarded by an if(), e.g.
> 
>   if (foo != 0)
>     x = 10;
>   else       // GUARD
>     y = 100; // BODY
> 
>   #ifdef BAR
>     blah ();  // NEXT
>   #endif

We have detect_preprocessor_logic which suppresses warnings when there's
preprocessor logic between BODY_EXPLOC and NEXT_STMT_EXPLOC, for cases
like this:

	if (flagA)
	  foo ();
	  ^ BODY_EXPLOC
      #if SOME_CONDITION_THAT_DOES_NOT_HOLD
	if (flagB)
      #endif
	  bar ();
	  ^ NEXT_STMT_EXPLOC

This currently requires that it be multiline preprocessor logic: there
must be 3 or more lines between BODY_EXPLOC and NEXT_STMT_EXPLOC for
this rejection heuristic to fire.

Perhaps we could tweak or reuse that heuristic, perhaps if there's an
entirely blank (or all whitespace) line separating them (given that this
warning is all about the "look" of the code).

> These bogus warnings are pre-existing, however (i.e. not caused by this
> patch).

(nods)   Fixing the false positives from libpng/bdwgc sounds like a
separate issue and thus a separate patch then.

[...snip...]

Re: [PATCH 1/3] Refactor entry point to -Wmisleading-indentation

[David Malcolm]

On Thu, 2015-06-18 at 11:41 -0400, Patrick Palka wrote:
> On Tue, Jun 9, 2015 at 1:31 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> > This patch refactors the entry point of -Wmisleading-indentation from:
> >
> >   void
> >   warn_for_misleading_indentation (location_t guard_loc,
> >                                    location_t body_loc,
> >                                    location_t next_stmt_loc,
> >                                    enum cpp_ttype next_tok_type,
> >                                    const char *guard_kind);
> >
> > to
> >
> >   struct token_indent_info
> >   {
> >     location_t location;
> >     cpp_ttype type;
> >     rid keyword;
> >   };
> >
> >   void
> >   warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> >                                    const token_indent_info &body_tinfo,
> >                                    const token_indent_info &next_tinfo);
> >
> > The purpose of this refactoring is to expose more information to the
> > -Wmisleading-indentation implementation to allow for more advanced
> > heuristics and for better coverage.
> >
> > (I decided to keep the usage of const references because nobody
> > seems to mind.  Also I added a new header file, c-indentation.h.)
> >
> > gcc/c-family/ChangeLog:
> >
> >         * c-indentation.h (struct token_indent_info): Define.
> >         (get_token_indent_info): Define.
> >         (warn_for_misleading_information): Declare.
> >         * c-common.h (warn_for_misleading_information): Remove.
> >         * c-identation.c (warn_for_misleading_indentation):
> >         Change declaration to take three token_indent_infos.  Adjust
> >         accordingly.
> >         * c-identation.c (should_warn_for_misleading_indentation):
> >         Likewise.  Bail out early if the body is a compound statement.
> >         (guard_tinfo_to_string): Define.
> >
> > gcc/c/ChangeLog:
> >
> >         * c-parser.c (c_parser_if_body): Take token_indent_info
> >         argument. Call warn_for_misleading_indentation even when the
> >         body is a semicolon.  Extract token_indent_infos corresponding
> >         to the guard, body and next tokens.  Adjust call to
> >         warn_for_misleading_indentation accordingly.
> >         (c_parser_else_body): Likewise.
> >         (c_parser_if_statement): Likewise.
> >         (c_parser_while_statement): Likewise.
> >         (c_parser_for_statement): Likewise.
> >
> > gcc/cp/ChangeLog:
> >
> >         * parser.c (cp_parser_selection_statement): Move handling of
> >         semicolon body to ...
> >         (cp_parser_implicitly_scoped_statement): .. here.  Call
> >         warn_for_misleading_indentation even when the body is a
> >         semicolon.  Extract token_indent_infos corresponding to the
> >         guard, body and next tokens.  Adjust call to
> >         warn_for_misleading_indentation accordingly.  Take
> >         token_indent_info argument.
> >         (cp_parser_already_scoped_statement): Likewise.
> >         (cp_parser_selection_statement, cp_parser_iteration_statement):
> >         Extract a token_indent_info corresponding to the guard token.
> 
> Pinging this series.

FWIW, they look reasonable to me; I'm not a reviewer.

Re: [PATCH 3/3] Improve -Wmissing-indentation heuristics

[Patrick Palka]

On Thu, Jun 18, 2015 at 12:31 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Tue, 2015-06-09 at 13:31 -0400, Patrick Palka wrote:
>> This patch improves the heuristics of the warning in a number of ways.
>> The improvements are hopefully adequately documented in the code
>> comments.
>>
>> The additions to the test case also highlight the improvements.
>>
>> I tested an earlier version of this patch on more than a dozen C code
>> bases.  I only found one class of bogus warnings yet emitted, in the
>> libpng and bdwgc projects.  These projects have a coding style which
>> indents code inside #ifdefs as if this code was guarded by an if(), e.g.
>>
>>   if (foo != 0)
>>     x = 10;
>>   else       // GUARD
>>     y = 100; // BODY
>>
>>   #ifdef BAR
>>     blah ();  // NEXT
>>   #endif
>
> We have detect_preprocessor_logic which suppresses warnings when there's
> preprocessor logic between BODY_EXPLOC and NEXT_STMT_EXPLOC, for cases
> like this:
>
>         if (flagA)
>           foo ();
>           ^ BODY_EXPLOC
>       #if SOME_CONDITION_THAT_DOES_NOT_HOLD
>         if (flagB)
>       #endif
>           bar ();
>           ^ NEXT_STMT_EXPLOC
>
> This currently requires that it be multiline preprocessor logic: there
> must be 3 or more lines between BODY_EXPLOC and NEXT_STMT_EXPLOC for
> this rejection heuristic to fire.

Oh I now see why it requires 3 or more lines: one line each for the
#if, #endif and for the

>
> Perhaps we could tweak or reuse that heuristic, perhaps if there's an
> entirely blank (or all whitespace) line separating them (given that this
> warning is all about the "look" of the code).

That makes sense.  What about just checking in
detect_preprocessor_logic() if there is > 1 line (instead of >= 3
lines) between the body and the next statement?  When that's the case,
then whatever is in between the start of the body must either be more
of the body (if it's a multi-line compound statement) or whitespace.
In either case we should not warn if the next statement is aligned
with the body.  Yet we will still rightfully warn on the following
code:

    if (foo)  // GUARD
      bar ();  // BODY
    #ifdef BAZ
      baz ();  // NEXT
    #endif

because there is just one line between the body and the next
statement. The user can add a line between the body and the next
statement to suppress the warning if it's bogus.

>
>> These bogus warnings are pre-existing, however (i.e. not caused by this
>> patch).
>
> (nods)   Fixing the false positives from libpng/bdwgc sounds like a
> separate issue and thus a separate patch then.
>
> [...snip...]
>
>

Re: [PATCH 1/3] Refactor entry point to -Wmisleading-indentation

[Patrick Palka]

On Thu, Jun 18, 2015 at 12:39 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Thu, 2015-06-18 at 11:41 -0400, Patrick Palka wrote:
>> On Tue, Jun 9, 2015 at 1:31 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> > This patch refactors the entry point of -Wmisleading-indentation from:
>> >
>> >   void
>> >   warn_for_misleading_indentation (location_t guard_loc,
>> >                                    location_t body_loc,
>> >                                    location_t next_stmt_loc,
>> >                                    enum cpp_ttype next_tok_type,
>> >                                    const char *guard_kind);
>> >
>> > to
>> >
>> >   struct token_indent_info
>> >   {
>> >     location_t location;
>> >     cpp_ttype type;
>> >     rid keyword;
>> >   };
>> >
>> >   void
>> >   warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
>> >                                    const token_indent_info &body_tinfo,
>> >                                    const token_indent_info &next_tinfo);
>> >
>> > The purpose of this refactoring is to expose more information to the
>> > -Wmisleading-indentation implementation to allow for more advanced
>> > heuristics and for better coverage.
>> >
>> > (I decided to keep the usage of const references because nobody
>> > seems to mind.  Also I added a new header file, c-indentation.h.)
>> >
>> > gcc/c-family/ChangeLog:
>> >
>> >         * c-indentation.h (struct token_indent_info): Define.
>> >         (get_token_indent_info): Define.
>> >         (warn_for_misleading_information): Declare.
>> >         * c-common.h (warn_for_misleading_information): Remove.
>> >         * c-identation.c (warn_for_misleading_indentation):
>> >         Change declaration to take three token_indent_infos.  Adjust
>> >         accordingly.
>> >         * c-identation.c (should_warn_for_misleading_indentation):
>> >         Likewise.  Bail out early if the body is a compound statement.
>> >         (guard_tinfo_to_string): Define.
>> >
>> > gcc/c/ChangeLog:
>> >
>> >         * c-parser.c (c_parser_if_body): Take token_indent_info
>> >         argument. Call warn_for_misleading_indentation even when the
>> >         body is a semicolon.  Extract token_indent_infos corresponding
>> >         to the guard, body and next tokens.  Adjust call to
>> >         warn_for_misleading_indentation accordingly.
>> >         (c_parser_else_body): Likewise.
>> >         (c_parser_if_statement): Likewise.
>> >         (c_parser_while_statement): Likewise.
>> >         (c_parser_for_statement): Likewise.
>> >
>> > gcc/cp/ChangeLog:
>> >
>> >         * parser.c (cp_parser_selection_statement): Move handling of
>> >         semicolon body to ...
>> >         (cp_parser_implicitly_scoped_statement): .. here.  Call
>> >         warn_for_misleading_indentation even when the body is a
>> >         semicolon.  Extract token_indent_infos corresponding to the
>> >         guard, body and next tokens.  Adjust call to
>> >         warn_for_misleading_indentation accordingly.  Take
>> >         token_indent_info argument.
>> >         (cp_parser_already_scoped_statement): Likewise.
>> >         (cp_parser_selection_statement, cp_parser_iteration_statement):
>> >         Extract a token_indent_info corresponding to the guard token.
>>
>> Pinging this series.
>
> FWIW, they look reasonable to me; I'm not a reviewer.

Thanks for going over them.

Re: [PATCH 3/3] Improve -Wmissing-indentation heuristics

[Patrick Palka]

On Thu, Jun 18, 2015 at 12:57 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Thu, Jun 18, 2015 at 12:31 PM, David Malcolm <dmalcolm@redhat.com> wrote:
>> On Tue, 2015-06-09 at 13:31 -0400, Patrick Palka wrote:
>>> This patch improves the heuristics of the warning in a number of ways.
>>> The improvements are hopefully adequately documented in the code
>>> comments.
>>>
>>> The additions to the test case also highlight the improvements.
>>>
>>> I tested an earlier version of this patch on more than a dozen C code
>>> bases.  I only found one class of bogus warnings yet emitted, in the
>>> libpng and bdwgc projects.  These projects have a coding style which
>>> indents code inside #ifdefs as if this code was guarded by an if(), e.g.
>>>
>>>   if (foo != 0)
>>>     x = 10;
>>>   else       // GUARD
>>>     y = 100; // BODY
>>>
>>>   #ifdef BAR
>>>     blah ();  // NEXT
>>>   #endif
>>
>> We have detect_preprocessor_logic which suppresses warnings when there's
>> preprocessor logic between BODY_EXPLOC and NEXT_STMT_EXPLOC, for cases
>> like this:
>>
>>         if (flagA)
>>           foo ();
>>           ^ BODY_EXPLOC
>>       #if SOME_CONDITION_THAT_DOES_NOT_HOLD
>>         if (flagB)
>>       #endif
>>           bar ();
>>           ^ NEXT_STMT_EXPLOC
>>
>> This currently requires that it be multiline preprocessor logic: there
>> must be 3 or more lines between BODY_EXPLOC and NEXT_STMT_EXPLOC for
>> this rejection heuristic to fire.
>
> Oh I now see why it requires 3 or more lines: one line each for the
> #if, #endif and for the
>
>>
>> Perhaps we could tweak or reuse that heuristic, perhaps if there's an
>> entirely blank (or all whitespace) line separating them (given that this
>> warning is all about the "look" of the code).
>
> That makes sense.  What about just checking in
> detect_preprocessor_logic() if there is > 1 line (instead of >= 3
> lines) between the body and the next statement?  When that's the case,
> then whatever is in between the start of the body must either be more
> of the body (if it's a multi-line compound statement) or whitespace.
> In either case we should not warn if the next statement is aligned
> with the body.

Actually, the body cannot be a compound statement because (with this
patch) we have already bailed out in that case.  So if there is > 1
line between the body and the next statement then there must be
whitespace... unless there are just more #ifdefs, e.g.

    if (foo)
      bar ();
    #ifdef A
    #ifdef B
      baz ();
    #endif
    #endif

We would want to warn here even though there are 2 lines, not 1 line,
in between the body and the next statement.  So we still have to
directly check for a whitespace line,  I think.

> Yet we will still rightfully warn on the following
> code:

>
>     if (foo)  // GUARD
>       bar ();  // BODY
>     #ifdef BAZ
>       baz ();  // NEXT
>     #endif
>
> because there is just one line between the body and the next
> statement. The user can add a line between the body and the next
> statement to suppress the warning if it's bogus.

I meant to say, a (empty) line between the body and the #ifdef.

>
>>
>>> These bogus warnings are pre-existing, however (i.e. not caused by this
>>> patch).
>>
>> (nods)   Fixing the false positives from libpng/bdwgc sounds like a
>> separate issue and thus a separate patch then.
>>
>> [...snip...]
>>
>>

Re: [PATCH 1/3] Refactor entry point to -Wmisleading-indentation

[Jeff Law]

On 06/18/2015 10:39 AM, David Malcolm wrote:
> On Thu, 2015-06-18 at 11:41 -0400, Patrick Palka wrote:
>> On Tue, Jun 9, 2015 at 1:31 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>>> This patch refactors the entry point of -Wmisleading-indentation from:
>>>
>>>    void
>>>    warn_for_misleading_indentation (location_t guard_loc,
>>>                                     location_t body_loc,
>>>                                     location_t next_stmt_loc,
>>>                                     enum cpp_ttype next_tok_type,
>>>                                     const char *guard_kind);
>>>
>>> to
>>>
>>>    struct token_indent_info
>>>    {
>>>      location_t location;
>>>      cpp_ttype type;
>>>      rid keyword;
>>>    };
>>>
>>>    void
>>>    warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
>>>                                     const token_indent_info &body_tinfo,
>>>                                     const token_indent_info &next_tinfo);
>>>
>>> The purpose of this refactoring is to expose more information to the
>>> -Wmisleading-indentation implementation to allow for more advanced
>>> heuristics and for better coverage.
>>>
>>> (I decided to keep the usage of const references because nobody
>>> seems to mind.  Also I added a new header file, c-indentation.h.)
>>>
>>> gcc/c-family/ChangeLog:
>>>
>>>          * c-indentation.h (struct token_indent_info): Define.
>>>          (get_token_indent_info): Define.
>>>          (warn_for_misleading_information): Declare.
>>>          * c-common.h (warn_for_misleading_information): Remove.
>>>          * c-identation.c (warn_for_misleading_indentation):
>>>          Change declaration to take three token_indent_infos.  Adjust
>>>          accordingly.
>>>          * c-identation.c (should_warn_for_misleading_indentation):
>>>          Likewise.  Bail out early if the body is a compound statement.
>>>          (guard_tinfo_to_string): Define.
>>>
>>> gcc/c/ChangeLog:
>>>
>>>          * c-parser.c (c_parser_if_body): Take token_indent_info
>>>          argument. Call warn_for_misleading_indentation even when the
>>>          body is a semicolon.  Extract token_indent_infos corresponding
>>>          to the guard, body and next tokens.  Adjust call to
>>>          warn_for_misleading_indentation accordingly.
>>>          (c_parser_else_body): Likewise.
>>>          (c_parser_if_statement): Likewise.
>>>          (c_parser_while_statement): Likewise.
>>>          (c_parser_for_statement): Likewise.
>>>
>>> gcc/cp/ChangeLog:
>>>
>>>          * parser.c (cp_parser_selection_statement): Move handling of
>>>          semicolon body to ...
>>>          (cp_parser_implicitly_scoped_statement): .. here.  Call
>>>          warn_for_misleading_indentation even when the body is a
>>>          semicolon.  Extract token_indent_infos corresponding to the
>>>          guard, body and next tokens.  Adjust call to
>>>          warn_for_misleading_indentation accordingly.  Take
>>>          token_indent_info argument.
>>>          (cp_parser_already_scoped_statement): Likewise.
>>>          (cp_parser_selection_statement, cp_parser_iteration_statement):
>>>          Extract a token_indent_info corresponding to the guard token.
>>
>> Pinging this series.
>
> FWIW, they look reasonable to me; I'm not a reviewer.
But as the implementer of the warning, your comments/thoughts are 
definitely helpful in the review process.

We've never worked too hard to find a way to formalize this into a set 
of policies and procedures, which is probably a mistake.

jeff

Re: [PATCH 3/3] Improve -Wmissing-indentation heuristics

[Jeff Law]

On 06/18/2015 10:57 AM, Patrick Palka wrote:
[ big snip ]
>>
>>> These bogus warnings are pre-existing, however (i.e. not caused by this
>>> patch).
>>
>> (nods)   Fixing the false positives from libpng/bdwgc sounds like a
>> separate issue and thus a separate patch then.
Agreed.

jeff

Re: [PATCH 1/3] Refactor entry point to -Wmisleading-indentation

[Jeff Law]

On 06/09/2015 11:31 AM, Patrick Palka wrote:
> This patch refactors the entry point of -Wmisleading-indentation from:
>
>    void
>    warn_for_misleading_indentation (location_t guard_loc,
>                                     location_t body_loc,
>                                     location_t next_stmt_loc,
>                                     enum cpp_ttype next_tok_type,
>                                     const char *guard_kind);
>
> to
>
>    struct token_indent_info
>    {
>      location_t location;
>      cpp_ttype type;
>      rid keyword;
>    };
>
>    void
>    warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
>                                     const token_indent_info &body_tinfo,
>                                     const token_indent_info &next_tinfo);
>
> The purpose of this refactoring is to expose more information to the
> -Wmisleading-indentation implementation to allow for more advanced
> heuristics and for better coverage.
>
> (I decided to keep the usage of const references because nobody
> seems to mind.  Also I added a new header file, c-indentation.h.)
>
> gcc/c-family/ChangeLog:
>
> 	* c-indentation.h (struct token_indent_info): Define.
> 	(get_token_indent_info): Define.
> 	(warn_for_misleading_information): Declare.
> 	* c-common.h (warn_for_misleading_information): Remove.
> 	* c-identation.c (warn_for_misleading_indentation):
> 	Change declaration to take three token_indent_infos.  Adjust
> 	accordingly.
> 	* c-identation.c (should_warn_for_misleading_indentation):
> 	Likewise.  Bail out early if the body is a compound statement.
> 	(guard_tinfo_to_string): Define.
>
> gcc/c/ChangeLog:
>
> 	* c-parser.c (c_parser_if_body): Take token_indent_info
> 	argument. Call warn_for_misleading_indentation even when the
> 	body is a semicolon.  Extract token_indent_infos corresponding
> 	to the guard, body and next tokens.  Adjust call to
> 	warn_for_misleading_indentation accordingly.
> 	(c_parser_else_body): Likewise.
> 	(c_parser_if_statement): Likewise.
> 	(c_parser_while_statement): Likewise.
> 	(c_parser_for_statement): Likewise.
>
> gcc/cp/ChangeLog:
>
> 	* parser.c (cp_parser_selection_statement): Move handling of
> 	semicolon body to ...
> 	(cp_parser_implicitly_scoped_statement): .. here.  Call
> 	warn_for_misleading_indentation even when the body is a
> 	semicolon.  Extract token_indent_infos corresponding to the
> 	guard, body and next tokens.  Adjust call to
> 	warn_for_misleading_indentation accordingly.  Take
> 	token_indent_info argument.
> 	(cp_parser_already_scoped_statement): Likewise.
> 	(cp_parser_selection_statement, cp_parser_iteration_statement):
> 	Extract a token_indent_info corresponding to the guard token.
The only question in my mind is bootstrap & regression testing.  From 
reading the thread for the earlier version of this patch I got the 
impression you had bootstrapped and regression tested earlier versions.

If you could confirm that you've bootstrapped and regression tested this 
version it'd be appreciated.  You can do it on the individual patches or 
the set as a whole.

Jeff

Re: [PATCH 2/3] Remove is_first_nonwhitespace_on_line(), instead improve get_visual_column()

[Jeff Law]

On 06/09/2015 11:31 AM, Patrick Palka wrote:
> This patch removes the function is_first_nonwhitespace_on_line() in
> favor of augmenting the function get_visual_column() to optionally
> return the visual column corresponding to the first non-whitespace character
> on the line.  Existing usage of is_first_nonwhitespace_on_line() can
> be trivially replaced by calling get_visual_column() and comparing *out
> with *first_nws.
>
> The rationale for this change is that in many cases it is better to use
> the visual column of the first non-whitespace character rather than the
> visual column of the token.  Consider:
>
>    if (p) {
>      foo (1);
>    } else       // GUARD
>      if (q)     // BODY
>        foo (2);
>      foo (3);   // NEXT
>
> Here, with current heuristics, we do not emit a warning because we
> notice that the visual columns of each token line up ("suggesting"
> autogenerated code).  Yet it is obvious that we should warn here because
> it misleadingly looks like the foo (3); statement is guarded by the
> else.
>
> If we instead consider the visual column of the first non-whitespace
> character on the guard line, the columns will not line up thus we will
> emit the warning.  This will be done in the next patch.
>
> gcc/c-family/ChangeLog:
>
> 	* c-indentation.c (get_visual_column): Add parameter first_nws,
> 	use it.  Update comment documenting the function.
> 	(is_first_nonwhitespace_on_line): Remove.
> 	(should_warn_for_misleading_indentation): Replace usage of
> 	of is_first_nonwhitespace_on_line with get_visual_column.
Same comment/question WRT testing as the prior patch.

OK once you've confirmed bootstrap & regression testing was completed 
successfully.

jeff

Re: [PATCH 3/3] Improve -Wmissing-indentation heuristics

[Jeff Law]

On 06/09/2015 11:31 AM, Patrick Palka wrote:
> This patch improves the heuristics of the warning in a number of ways.
> The improvements are hopefully adequately documented in the code
> comments.
>
> The additions to the test case also highlight the improvements.
>
> I tested an earlier version of this patch on more than a dozen C code
> bases.  I only found one class of bogus warnings yet emitted, in the
> libpng and bdwgc projects.  These projects have a coding style which
> indents code inside #ifdefs as if this code was guarded by an if(), e.g.
>
>    if (foo != 0)
>      x = 10;
>    else       // GUARD
>      y = 100; // BODY
>
>    #ifdef BAR
>      blah ();  // NEXT
>    #endif
>
> These bogus warnings are pre-existing, however (i.e. not caused by this
> patch).
>
> 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.
OK after confirming a successful bootstrap & regression test.

jeff

Re: [PATCH 1/3] Refactor entry point to -Wmisleading-indentation

[Patrick Palka]

On Mon, Jun 22, 2015 at 1:29 PM, Jeff Law <law@redhat.com> wrote:
> On 06/09/2015 11:31 AM, Patrick Palka wrote:
>>
>> This patch refactors the entry point of -Wmisleading-indentation from:
>>
>>    void
>>    warn_for_misleading_indentation (location_t guard_loc,
>>                                     location_t body_loc,
>>                                     location_t next_stmt_loc,
>>                                     enum cpp_ttype next_tok_type,
>>                                     const char *guard_kind);
>>
>> to
>>
>>    struct token_indent_info
>>    {
>>      location_t location;
>>      cpp_ttype type;
>>      rid keyword;
>>    };
>>
>>    void
>>    warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
>>                                     const token_indent_info &body_tinfo,
>>                                     const token_indent_info &next_tinfo);
>>
>> The purpose of this refactoring is to expose more information to the
>> -Wmisleading-indentation implementation to allow for more advanced
>> heuristics and for better coverage.
>>
>> (I decided to keep the usage of const references because nobody
>> seems to mind.  Also I added a new header file, c-indentation.h.)
>>
>> gcc/c-family/ChangeLog:
>>
>>         * c-indentation.h (struct token_indent_info): Define.
>>         (get_token_indent_info): Define.
>>         (warn_for_misleading_information): Declare.
>>         * c-common.h (warn_for_misleading_information): Remove.
>>         * c-identation.c (warn_for_misleading_indentation):
>>         Change declaration to take three token_indent_infos.  Adjust
>>         accordingly.
>>         * c-identation.c (should_warn_for_misleading_indentation):
>>         Likewise.  Bail out early if the body is a compound statement.
>>         (guard_tinfo_to_string): Define.
>>
>> gcc/c/ChangeLog:
>>
>>         * c-parser.c (c_parser_if_body): Take token_indent_info
>>         argument. Call warn_for_misleading_indentation even when the
>>         body is a semicolon.  Extract token_indent_infos corresponding
>>         to the guard, body and next tokens.  Adjust call to
>>         warn_for_misleading_indentation accordingly.
>>         (c_parser_else_body): Likewise.
>>         (c_parser_if_statement): Likewise.
>>         (c_parser_while_statement): Likewise.
>>         (c_parser_for_statement): Likewise.
>>
>> gcc/cp/ChangeLog:
>>
>>         * parser.c (cp_parser_selection_statement): Move handling of
>>         semicolon body to ...
>>         (cp_parser_implicitly_scoped_statement): .. here.  Call
>>         warn_for_misleading_indentation even when the body is a
>>         semicolon.  Extract token_indent_infos corresponding to the
>>         guard, body and next tokens.  Adjust call to
>>         warn_for_misleading_indentation accordingly.  Take
>>         token_indent_info argument.
>>         (cp_parser_already_scoped_statement): Likewise.
>>         (cp_parser_selection_statement, cp_parser_iteration_statement):
>>         Extract a token_indent_info corresponding to the guard token.
>
> The only question in my mind is bootstrap & regression testing.  From
> reading the thread for the earlier version of this patch I got the
> impression you had bootstrapped and regression tested earlier versions.
>
> If you could confirm that you've bootstrapped and regression tested this
> version it'd be appreciated.  You can do it on the individual patches or the
> set as a whole.

I think I successfully bootstrapped + regtested this exact version but
I'm not sure.  I was going to do so again before committing anyway.
I will fire off a build tonight and confirm the results tomorrow.

>
> Jeff
>
>

Re: [PATCH 1/3] Refactor entry point to -Wmisleading-indentation

[Patrick Palka]

On Mon, Jun 22, 2015 at 2:56 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Mon, Jun 22, 2015 at 1:29 PM, Jeff Law <law@redhat.com> wrote:
>> On 06/09/2015 11:31 AM, Patrick Palka wrote:
>>>
>>> This patch refactors the entry point of -Wmisleading-indentation from:
>>>
>>>    void
>>>    warn_for_misleading_indentation (location_t guard_loc,
>>>                                     location_t body_loc,
>>>                                     location_t next_stmt_loc,
>>>                                     enum cpp_ttype next_tok_type,
>>>                                     const char *guard_kind);
>>>
>>> to
>>>
>>>    struct token_indent_info
>>>    {
>>>      location_t location;
>>>      cpp_ttype type;
>>>      rid keyword;
>>>    };
>>>
>>>    void
>>>    warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
>>>                                     const token_indent_info &body_tinfo,
>>>                                     const token_indent_info &next_tinfo);
>>>
>>> The purpose of this refactoring is to expose more information to the
>>> -Wmisleading-indentation implementation to allow for more advanced
>>> heuristics and for better coverage.
>>>
>>> (I decided to keep the usage of const references because nobody
>>> seems to mind.  Also I added a new header file, c-indentation.h.)
>>>
>>> gcc/c-family/ChangeLog:
>>>
>>>         * c-indentation.h (struct token_indent_info): Define.
>>>         (get_token_indent_info): Define.
>>>         (warn_for_misleading_information): Declare.
>>>         * c-common.h (warn_for_misleading_information): Remove.
>>>         * c-identation.c (warn_for_misleading_indentation):
>>>         Change declaration to take three token_indent_infos.  Adjust
>>>         accordingly.
>>>         * c-identation.c (should_warn_for_misleading_indentation):
>>>         Likewise.  Bail out early if the body is a compound statement.
>>>         (guard_tinfo_to_string): Define.
>>>
>>> gcc/c/ChangeLog:
>>>
>>>         * c-parser.c (c_parser_if_body): Take token_indent_info
>>>         argument. Call warn_for_misleading_indentation even when the
>>>         body is a semicolon.  Extract token_indent_infos corresponding
>>>         to the guard, body and next tokens.  Adjust call to
>>>         warn_for_misleading_indentation accordingly.
>>>         (c_parser_else_body): Likewise.
>>>         (c_parser_if_statement): Likewise.
>>>         (c_parser_while_statement): Likewise.
>>>         (c_parser_for_statement): Likewise.
>>>
>>> gcc/cp/ChangeLog:
>>>
>>>         * parser.c (cp_parser_selection_statement): Move handling of
>>>         semicolon body to ...
>>>         (cp_parser_implicitly_scoped_statement): .. here.  Call
>>>         warn_for_misleading_indentation even when the body is a
>>>         semicolon.  Extract token_indent_infos corresponding to the
>>>         guard, body and next tokens.  Adjust call to
>>>         warn_for_misleading_indentation accordingly.  Take
>>>         token_indent_info argument.
>>>         (cp_parser_already_scoped_statement): Likewise.
>>>         (cp_parser_selection_statement, cp_parser_iteration_statement):
>>>         Extract a token_indent_info corresponding to the guard token.
>>
>> The only question in my mind is bootstrap & regression testing.  From
>> reading the thread for the earlier version of this patch I got the
>> impression you had bootstrapped and regression tested earlier versions.
>>
>> If you could confirm that you've bootstrapped and regression tested this
>> version it'd be appreciated.  You can do it on the individual patches or the
>> set as a whole.
>
> I think I successfully bootstrapped + regtested this exact version but
> I'm not sure.  I was going to do so again before committing anyway.
> I will fire off a build tonight and confirm the results tomorrow.

Bootstrap + regtest on x86_64-linux-gnu was successful with no new regressions.

>
>>
>> Jeff
>>
>>

Re: [PATCH 1/3] Refactor entry point to -Wmisleading-indentation

[Patrick Palka]

On Tue, Jun 23, 2015 at 3:05 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Mon, Jun 22, 2015 at 2:56 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> On Mon, Jun 22, 2015 at 1:29 PM, Jeff Law <law@redhat.com> wrote:
>>> On 06/09/2015 11:31 AM, Patrick Palka wrote:
>>>>
>>>> This patch refactors the entry point of -Wmisleading-indentation from:
>>>>
>>>>    void
>>>>    warn_for_misleading_indentation (location_t guard_loc,
>>>>                                     location_t body_loc,
>>>>                                     location_t next_stmt_loc,
>>>>                                     enum cpp_ttype next_tok_type,
>>>>                                     const char *guard_kind);
>>>>
>>>> to
>>>>
>>>>    struct token_indent_info
>>>>    {
>>>>      location_t location;
>>>>      cpp_ttype type;
>>>>      rid keyword;
>>>>    };
>>>>
>>>>    void
>>>>    warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
>>>>                                     const token_indent_info &body_tinfo,
>>>>                                     const token_indent_info &next_tinfo);
>>>>
>>>> The purpose of this refactoring is to expose more information to the
>>>> -Wmisleading-indentation implementation to allow for more advanced
>>>> heuristics and for better coverage.
>>>>
>>>> (I decided to keep the usage of const references because nobody
>>>> seems to mind.  Also I added a new header file, c-indentation.h.)
>>>>
>>>> gcc/c-family/ChangeLog:
>>>>
>>>>         * c-indentation.h (struct token_indent_info): Define.
>>>>         (get_token_indent_info): Define.
>>>>         (warn_for_misleading_information): Declare.
>>>>         * c-common.h (warn_for_misleading_information): Remove.
>>>>         * c-identation.c (warn_for_misleading_indentation):
>>>>         Change declaration to take three token_indent_infos.  Adjust
>>>>         accordingly.
>>>>         * c-identation.c (should_warn_for_misleading_indentation):
>>>>         Likewise.  Bail out early if the body is a compound statement.
>>>>         (guard_tinfo_to_string): Define.
>>>>
>>>> gcc/c/ChangeLog:
>>>>
>>>>         * c-parser.c (c_parser_if_body): Take token_indent_info
>>>>         argument. Call warn_for_misleading_indentation even when the
>>>>         body is a semicolon.  Extract token_indent_infos corresponding
>>>>         to the guard, body and next tokens.  Adjust call to
>>>>         warn_for_misleading_indentation accordingly.
>>>>         (c_parser_else_body): Likewise.
>>>>         (c_parser_if_statement): Likewise.
>>>>         (c_parser_while_statement): Likewise.
>>>>         (c_parser_for_statement): Likewise.
>>>>
>>>> gcc/cp/ChangeLog:
>>>>
>>>>         * parser.c (cp_parser_selection_statement): Move handling of
>>>>         semicolon body to ...
>>>>         (cp_parser_implicitly_scoped_statement): .. here.  Call
>>>>         warn_for_misleading_indentation even when the body is a
>>>>         semicolon.  Extract token_indent_infos corresponding to the
>>>>         guard, body and next tokens.  Adjust call to
>>>>         warn_for_misleading_indentation accordingly.  Take
>>>>         token_indent_info argument.
>>>>         (cp_parser_already_scoped_statement): Likewise.
>>>>         (cp_parser_selection_statement, cp_parser_iteration_statement):
>>>>         Extract a token_indent_info corresponding to the guard token.
>>>
>>> The only question in my mind is bootstrap & regression testing.  From
>>> reading the thread for the earlier version of this patch I got the
>>> impression you had bootstrapped and regression tested earlier versions.
>>>
>>> If you could confirm that you've bootstrapped and regression tested this
>>> version it'd be appreciated.  You can do it on the individual patches or the
>>> set as a whole.
>>
>> I think I successfully bootstrapped + regtested this exact version but
>> I'm not sure.  I was going to do so again before committing anyway.
>> I will fire off a build tonight and confirm the results tomorrow.
>
> Bootstrap + regtest on x86_64-linux-gnu was successful with no new regressions.
>
>>
>>>
>>> Jeff
>>>
>>>

Hi Jeff.  Sorry for the late reply.  I would like to commit this
series now (after another bootstrap + regtest cycle) to work on
further refinements to this warning.  You clearly approved the last
two out of three patches, but it is not clear whether you approved
this first patch.  Could you clarify your stance on this patch?
Thanks again for reviewing.

Re: [PATCH 1/3] Refactor entry point to -Wmisleading-indentation

[Jeff Law]

On 07/27/2015 05:12 PM, Patrick Palka wrote:
>>>>
>>>> The only question in my mind is bootstrap & regression testing.  From
>>>> reading the thread for the earlier version of this patch I got the
>>>> impression you had bootstrapped and regression tested earlier versions.
>>>>
>>>> If you could confirm that you've bootstrapped and regression tested this
>>>> version it'd be appreciated.  You can do it on the individual patches or the
>>>> set as a whole.
>>>
>>> I think I successfully bootstrapped + regtested this exact version but
>>> I'm not sure.  I was going to do so again before committing anyway.
>>> I will fire off a build tonight and confirm the results tomorrow.
>>
>> Bootstrap + regtest on x86_64-linux-gnu was successful with no new regressions.
>>
>>>
>>>>
>>>> Jeff
>>>>
>>>>
>
> Hi Jeff.  Sorry for the late reply.  I would like to commit this
> series now (after another bootstrap + regtest cycle) to work on
> further refinements to this warning.  You clearly approved the last
> two out of three patches, but it is not clear whether you approved
> this first patch.  Could you clarify your stance on this patch?
> Thanks again for reviewing.
Sorry for not being explicit.  Yes, patch #1 is fine with the bootstrap 
& regression test done.

Thanks,
jeff