diff mbox

Re: [RFC stage 1] Proposed new warning: -Wmisleading-indentation

Message ID 5536936F.8090004@gmail.com
State New
Headers show

Commit Message

Manuel López-Ibáñez April 21, 2015, 6:14 p.m. UTC
On 21/04/15 18:07, David Malcolm wrote:
>
> I have the patch working now for the C++ frontend.  Am attaching the
> work-in-progress (sans ChangeLog).  This one (v2) bootstrapped and
> regrtested on x86_64-unknown-linux-gnu (Fedora 20), with:
>    63 new "PASS" results in gcc.sum
>    189 new "PASS" results in g++.sum
> for the new test cases (relative to a control build of r222248).
>

I still do not understand why you need so much complexity as I explained here: 
https://gcc.gnu.org/ml/gcc-patches/2015-04/msg00830.html

The attached patch passes all your tests except Wmisleading-indentation-3.c, 
which warns only once instead of two times (it doesn't seem a big loss to me), 
and Wmisleading-indentation-7.c which I did not bother to implement but it is 
straightforward application of the if-case to the else-case.

Perhaps I'm missing something that is not reflected in your tests?

BTW, the start-up cost of GCC is not negligible, thus grouping similar 
testcases in a single file may pay off in the long term. Many small files also 
tend to slow down VC tools. It also makes harder to see what is tested and what 
is missing.

Cheers,

Manuel.

Comments

David Malcolm April 21, 2015, 11:28 p.m. UTC | #1
On Tue, 2015-04-21 at 20:14 +0200, Manuel López-Ibáñez wrote:
> On 21/04/15 18:07, David Malcolm wrote:
> >
> > I have the patch working now for the C++ frontend.  Am attaching the
> > work-in-progress (sans ChangeLog).  This one (v2) bootstrapped and
> > regrtested on x86_64-unknown-linux-gnu (Fedora 20), with:
> >    63 new "PASS" results in gcc.sum
> >    189 new "PASS" results in g++.sum
> > for the new test cases (relative to a control build of r222248).
> >
> 
> I still do not understand why you need so much complexity as I explained here: 
> https://gcc.gnu.org/ml/gcc-patches/2015-04/msg00830.html
> 
> The attached patch passes all your tests except Wmisleading-indentation-3.c, 
> which warns only once instead of two times (it doesn't seem a big loss to me), 
> and Wmisleading-indentation-7.c which I did not bother to implement but it is 
> straightforward application of the if-case to the else-case.

Aha!   Thanks.  Your approach is much simpler, and likely much faster.

> Perhaps I'm missing something that is not reflected in your tests?

No, mostly just my lack of expertise on the frontend :)

> BTW, the start-up cost of GCC is not negligible, thus grouping similar 
> testcases in a single file may pay off in the long term. Many small files also 
> tend to slow down VC tools. It also makes harder to see what is tested and what 
> is missing.

OK.  I'll finish up your version of the patch, and consolidate the
testcases.

Thanks
Dave
diff mbox

Patch

Index: c/c-parser.c
===================================================================
--- c/c-parser.c	(revision 222087)
+++ c/c-parser.c	(working copy)
@@ -5174,20 +5174,34 @@  c_parser_c99_block_statement (c_parser *
   location_t loc = c_parser_peek_token (parser)->location;
   c_parser_statement (parser);
   return c_end_compound_stmt (loc, block, flag_isoc99);
 }
 
+static void
+warn_for_misleading_indentation (location_t guard_loc, location_t body_loc, location_t next_stmt_loc,
+			     const char *s)
+{
+  if (LOCATION_FILE (next_stmt_loc) == LOCATION_FILE (body_loc)
+      && (LOCATION_LINE (next_stmt_loc) == LOCATION_LINE (body_loc)
+	  || (LOCATION_LINE (next_stmt_loc) > LOCATION_LINE (body_loc)
+	      && LOCATION_COLUMN (next_stmt_loc) == LOCATION_COLUMN (body_loc))))
+    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", s);
+}
+
 /* Parse the body of an if statement.  This is just parsing a
    statement but (a) it is a block in C99, (b) we track whether the
    body is an if statement for the sake of -Wparentheses warnings, (c)
    we handle an empty body specially for the sake of -Wempty-body
    warnings, and (d) we call parser_compound_statement directly
    because c_parser_statement_after_labels resets
    parser->in_if_block.  */
 
 static tree
-c_parser_if_body (c_parser *parser, bool *if_p)
+c_parser_if_body (c_parser *parser, bool *if_p, location_t if_loc)
 {
   tree block = c_begin_compound_stmt (flag_isoc99);
   location_t body_loc = c_parser_peek_token (parser)->location;
   c_parser_all_labels (parser);
   *if_p = c_parser_next_token_is_keyword (parser, RID_IF);
@@ -5201,11 +5215,16 @@  c_parser_if_body (c_parser *parser, bool
 		    "suggest braces around empty body in an %<if%> statement");
     }
   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);
+    {
+      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, "if");
+    }
+
   return c_end_compound_stmt (body_loc, block, flag_isoc99);
 }
 
 /* Parse the else body of an if statement.  This is just parsing a
    statement but (a) it is a block in C99, (b) we handle an empty body
@@ -5248,10 +5267,11 @@  c_parser_if_statement (c_parser *parser)
   tree first_body, second_body;
   bool in_if_block;
   tree if_stmt;
 
   gcc_assert (c_parser_next_token_is_keyword (parser, RID_IF));
+  location_t if_loc = c_parser_peek_token (parser)->location;
   c_parser_consume_token (parser);
   block = c_begin_compound_stmt (flag_isoc99);
   loc = c_parser_peek_token (parser)->location;
   cond = c_parser_paren_condition (parser);
   if (flag_cilkplus && contains_cilk_spawn_stmt (cond))
@@ -5259,11 +5279,11 @@  c_parser_if_statement (c_parser *parser)
       error_at (loc, "if statement cannot contain %<Cilk_spawn%>");
       cond = error_mark_node;
     }
   in_if_block = parser->in_if_block;
   parser->in_if_block = true;
-  first_body = c_parser_if_body (parser, &first_if);
+  first_body = c_parser_if_body (parser, &first_if, if_loc);
   parser->in_if_block = in_if_block;
   if (c_parser_next_token_is_keyword (parser, RID_ELSE))
     {
       c_parser_consume_token (parser);
       second_body = c_parser_else_body (parser);
@@ -5344,10 +5364,11 @@  static void
 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;
   c_parser_consume_token (parser);
   block = c_begin_compound_stmt (flag_isoc99);
   loc = c_parser_peek_token (parser)->location;
   cond = c_parser_paren_condition (parser);
   if (check_no_cilk (cond,
@@ -5360,11 +5381,17 @@  c_parser_while_statement (c_parser *pars
 		   annot_expr_ivdep_kind));
   save_break = c_break_label;
   c_break_label = NULL_TREE;
   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;
   body = c_parser_c99_block_statement (parser);
+  warn_for_misleading_indentation (while_loc, body_loc, c_parser_peek_token (parser)->location, "while");
+
   c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label, true);
   add_stmt (c_end_compound_stmt (loc, block, flag_isoc99));
   c_break_label = save_break;
   c_cont_label = save_cont;
 }
@@ -5638,11 +5665,17 @@  c_parser_for_statement (c_parser *parser
     }
   save_break = c_break_label;
   c_break_label = NULL_TREE;
   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;
   body = c_parser_c99_block_statement (parser);
+  warn_for_misleading_indentation (for_loc, body_loc, c_parser_peek_token (parser)->location, "for");
+
   if (is_foreach_statement)
     objc_finish_foreach_loop (loc, object_expression, collection_expression, body, c_break_label, c_cont_label);
   else
     c_finish_loop (loc, cond, incr, body, c_break_label, c_cont_label, true);
   add_stmt (c_end_compound_stmt (loc, block, flag_isoc99 || c_dialect_objc ()));