Re: More empty body checks

[Mike Stump <mrs at apple dot com>]

On May 6, 2007, at 4:26 PM, Mark Mitchell wrote:
> Typo: "immediately".

Fixed.

> Write as:
>
>   a warning for:
>
>      while (...);
>
>   and for:
>
>      for (...);

Fixed.

> Typo: space between "token" and "might".

Fixed.

> > +  close_paren = cp_lexer_peek_token (parser->lexer);
> > +  if (close_paren->type != CPP_CLOSE_PAREN)
> > +    return;
>
> This should be folded into the callers, to avoid the redundant token
> peeking:
>
>   if (cp_parser_require (CPP_CLOSE_PAREN))
>     check_empty_body (...);

Ah, but they already do this.  The problem is that I need to hold onto  
the token or the location information at least after that point, so, I  
need to run before it's required, as otherwise it would be consumed.   
If you're worried about speed, I could skip the call if  
warn_empty_body is not in effect, just let me know.  I didn't do that  
as I'd like to do the preexisting empty body checks this way and put  
them in -Wall, so the effects on speed the way I'd recommend running  
the compiler would be negative.

> More efficient as:
>
>   token = cp_lexer_peek_nth_token (parser->lexer, 2);
>   if (token->type != CPP_SEMICOLON
>        || (token->flags & PREV_WHITE))
>     return;
>   semi_loc = expand_location (...)
>   ...

Fixed.

> > +    warning (OPT_Wempty_body,
> > +	     "suggest a space before %<;%> or explicit braces around  
> > empty "
> > +	     "body in %<%s%> statement",
> > +	     type);
>
> You also need to update doc/invoke.texi to reflect that you're warning
> about this in C++ for for/while statements.

Fixed.

Ideally, it would be nice to fix up all the existing empty body code  
to use this style and flesh out this code for C as well...

Thanks for the review.

Ok?

Doing diffs in .:
--- ./cp/parser.c.~1~	2007-04-13 10:05:59.000000000 -0700
+++ ./cp/parser.c	2007-05-07 14:49:02.000000000 -0700
@@ -7065,6 +7065,51 @@ cp_parser_condition (cp_parser* parser)
   return cp_parser_expression (parser, /*cast_p=*/false);
 }

+/* We check for a ) immediately followed by ; with no whitespacing
+   between.  This is used to issue a warning for:
+
+     while (...);
+
+   and:
+
+     for (...);
+
+   as the semicolon is probably extraneous.
+
+   On parse errors, the next token might not be a ), so do nothing in
+   that case. */
+
+static void
+check_empty_body (cp_parser* parser, const char* type)
+{
+  cp_token *token;
+  cp_token *close_paren;
+  expanded_location close_loc;
+  expanded_location semi_loc;
+
+  close_paren = cp_lexer_peek_token (parser->lexer);
+  if (close_paren->type != CPP_CLOSE_PAREN)
+    return;
+
+  close_loc = expand_location (close_paren->location);
+  token = cp_lexer_peek_nth_token (parser->lexer, 2);
+
+  if (token->type != CPP_SEMICOLON
+      || (token->flags & PREV_WHITE))
+    return;
+
+  semi_loc =  expand_location (token->location);
+  if (close_loc.line == semi_loc.line
+#ifdef USE_MAPPED_LOCATION
+      && close_loc.column+1 == semi_loc.column
+#endif
+      )
+    warning (OPT_Wempty_body,
+	     "suggest a space before %<;%> or explicit braces around empty "
+	     "body in %<%s%> statement",
+	     type);
+}
+
 /* Parse an iteration-statement.

    iteration-statement:
@@ -7107,6 +7152,7 @@ cp_parser_iteration_statement (cp_parser
 	/* Parse the condition.  */
 	condition = cp_parser_condition (parser);
 	finish_while_stmt_cond (condition, statement);
+	check_empty_body (parser, "while");
 	/* Look for the `)'.  */
 	cp_parser_require (parser, CPP_CLOSE_PAREN, "`)'");
 	/* Parse the dependent statement.  */
@@ -7168,6 +7214,7 @@ cp_parser_iteration_statement (cp_parser
 	if (cp_lexer_next_token_is_not (parser->lexer, CPP_CLOSE_PAREN))
 	  expression = cp_parser_expression (parser, /*cast_p=*/false);
 	finish_for_expr (expression, statement);
+	check_empty_body (parser, "for");
 	/* Look for the `)'.  */
 	cp_parser_require (parser, CPP_CLOSE_PAREN, "`)'");

--- ./doc/invoke.texi.~1~	2007-05-02 14:59:39.000000000 -0700
+++ ./doc/invoke.texi	2007-05-07 14:56:30.000000000 -0700
@@ -3157,6 +3157,11 @@ An empty body occurs in an @samp{if}, @s
 @samp{do while} statement. This warning can be independently
 controlled by @option{-Wempty-body}.

+@item @r{(C++ only)}
+An empty body occurs in a @samp{while} or @samp{for} statement with no
+whitespacing before the semicolon. This warning can be independently
+controlled by @option{-Wempty-body}.
+
 @item
 A pointer is compared against integer zero with @samp{<}, @samp{<=},
 @samp{>}, or @samp{>=}.
@@ -3421,8 +3426,10 @@ to them.

 @item -Wempty-body
 @opindex Wempty-body
-An empty body occurs in an @samp{if}, @samp{else} or @samp{do while}
-statement.  This warning is also enabled by @option{-Wextra}.
+Warn if an empty body occurs in an @samp{if}, @samp{else} or @samp{do
+while} statement.  Additionally, in C++, warn when an empty body occurs
+in a @samp{while} or @samp{for} statement with no whitespacing before
+the semicolon.  This warning is also enabled by @option{-Wextra}.

 @item -Wsign-compare
 @opindex Wsign-compare
--- ./testsuite/g++.old-deja/g++.mike/empty.C.~1~	2007-04-25  
17:52:04.000000000 -0700
+++ ./testsuite/g++.old-deja/g++.mike/empty.C	2007-04-25  
17:51:39.000000000 -0700
@@ -0,0 +1,25 @@
+// { dg-options "-W" }
+
+#define NOPE
+
+void foo() {
+  while (1); /* { dg-error "suggest a space before " } */
+    {
+    }
+  for (;;); /* { dg-error "suggest a space before " } */
+    {
+    }
+  while (1)
+    ;
+  for (;;)
+    ;
+  while (1) ;
+  for (;;) ;
+  /* These two work when using mapped locations */
+  while (1) NOPE; /* { dg-bogus "suggest a space before "  
"suggest" { xfail *-*-* } } */
+  for (;;) NOPE; /* { dg-bogus "suggest a space before "  
"suggest" { xfail *-*-* } } */
+  while (1)
+    NOPE;
+  for (;;)
+    NOPE;
+}
--------------