[libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)

[libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)

[Jakub Jelinek]

Hi!

In 4.3 Mike Stump extended -Wempty-body warning, such that C++ FE (no idea
why only C++ FE and not C FE) warns about:
while (cond);
for (;;);
but still doesn't about:
#define EMPTY
while (cond) ;
while (cond)/*EMPTY*/;
while (cond)
;
while (cond)EMPTY;
Unfortunately the way it is implemented is horribly fragile and not stable
with/without -save-temps (or ccache or anything that goes preprocesses
separately from compilation).
1) With -save-temps, warning is issued for
#define EMPTY
while (cond)EMPTY;
when that wasn't intended.
2) With -save-temps, warning isn't issued if while (cond); comes from a
macro.
While 2) could be fixed as pointed by Manuel in the PR using starting/ending
location in location_t, 1) can't without libcpp being aware of it.

So, this patch changes libcpp to set a token flag on CPP_CLOSE_PAREN
if it was immediately followed in the source by semicolon (as if
we had ); token, just that 2 tokens still come from libcpp, with the
first one with a special PAREN_SEMICOLON flag) and uses that new flag
in C++ parser's check_empty_body (to determine if the warning should be
printed or not) and in c-ppoutput.c (to print extra space between ) ; if
needed).

The reason why I haven't just added PREV_WHITE flag in that case is that
I don't want to affect stringification and it would need to be handled
in many places within libcpp.

Ok for trunk if bootstrap/regtest passes?

2008-11-10  Jakub Jelinek  <jakub@redhat.com>

	PR c++/36478
libcpp/
	* include/cpplib.h (PURE_ZERO): Document that it is valid only on
	CPP_NUMBER.
	(PAREN_SEMICOLON): Define.
	* lex.c (_cpp_lex_direct): Set PAREN_SEMICOLON flag on CPP_CLOSE_PAREN
	if immediately followed by semicolon.
gcc/cp/
	* parser.c (check_empty_body): Check PAREN_SEMICOLON flag on
	CPP_CLOSE_PAREN instead of comparing source lines and columns of the
	tokens.
gcc/
	* c-ppoutput.c (scan_translation_unit): Output space between ) and ;
	if PAREN_SEMICOLON isn't set on CPP_CLOSE_PAREN.
gcc/testsuite/
	* g++.dg/warn/Wempty-body-1.C: New test.
	* g++.dg/warn/Wempty-body-2.C: New test.

--- libcpp/include/cpplib.h.jj	2008-10-23 13:22:48.000000000 +0200
+++ libcpp/include/cpplib.h	2008-11-10 11:29:02.000000000 +0100
@@ -176,8 +176,11 @@ struct cpp_string GTY(())
 #define NAMED_OP	(1 << 4) /* C++ named operators.  */
 #define NO_EXPAND	(1 << 5) /* Do not macro-expand this token.  */
 #define BOL		(1 << 6) /* Token at beginning of line.  */
-#define PURE_ZERO	(1 << 7) /* Single 0 digit, used by the C++ frontend,
-				    set in c-lex.c.  */
+#define PURE_ZERO	(1 << 7) /* On CPP_NUMBER, single 0 digit, used by the
+				    C++ frontend, set in c-lex.c.  */
+#define PAREN_SEMICOLON	(1 << 7) /* On CPP_CLOSE_PAREN if immediately
+				    followed by CPP_SEMICOLON, with no
+				    intervening whitespace or macros.  */
 
 /* Specify which field, if any, of the cpp_token union is used.  */
 
--- libcpp/lex.c.jj	2008-10-23 13:22:48.000000000 +0200
+++ libcpp/lex.c	2008-11-10 11:32:28.000000000 +0100
@@ -1319,7 +1319,11 @@ _cpp_lex_direct (cpp_reader *pfile)
     case '~': result->type = CPP_COMPL; break;
     case ',': result->type = CPP_COMMA; break;
     case '(': result->type = CPP_OPEN_PAREN; break;
-    case ')': result->type = CPP_CLOSE_PAREN; break;
+    case ')':
+      result->type = CPP_CLOSE_PAREN;
+      if (*buffer->cur == ';')
+	result->flags |= PAREN_SEMICOLON;
+      break;
     case '[': result->type = CPP_OPEN_SQUARE; break;
     case ']': result->type = CPP_CLOSE_SQUARE; break;
     case '{': result->type = CPP_OPEN_BRACE; break;
--- gcc/cp/parser.c.jj	2008-11-10 10:28:19.000000000 +0100
+++ gcc/cp/parser.c	2008-11-10 11:34:14.000000000 +0100
@@ -7446,27 +7446,22 @@ check_empty_body (cp_parser* parser, con
 {
   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)
+  if (close_paren->type != CPP_CLOSE_PAREN
+      || (close_paren->flags & PAREN_SEMICOLON) == 0)
     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
-      && close_loc.column+1 == semi_loc.column)
-    warning (OPT_Wempty_body,
-	     "suggest a space before %<;%> or explicit braces around empty "
-	     "body in %<%s%> statement",
-	     type);
+  warning (OPT_Wempty_body,
+	   "suggest a space before %<;%> or explicit braces around empty "
+	   "body in %<%s%> statement",
+	   type);
 }
 
 /* Parse an iteration-statement.
--- gcc/c-ppoutput.c.jj	2008-10-23 13:21:41.000000000 +0200
+++ gcc/c-ppoutput.c	2008-11-10 14:07:28.000000000 +0100
@@ -188,9 +188,21 @@ scan_translation_unit (cpp_reader *pfile
 	      || (print.prev
 		  && cpp_avoid_paste (pfile, print.prev, token))
 	      || (print.prev == NULL && token->type == CPP_HASH))
-	    putc (' ', print.outf);
+	    {
+	      putc (' ', print.outf);
+	      print.prev = NULL;
+	    }
 	}
       else if (token->flags & PREV_WHITE)
+	{
+	  putc (' ', print.outf);
+	  print.prev = NULL;
+	}
+      if (token->type == CPP_SEMICOLON
+	  && print.prev != NULL
+	  && print.prev->type == CPP_CLOSE_PAREN
+	  && !(print.prev->flags & PAREN_SEMICOLON)
+	  && cpp_get_options (parse_in)->lang != CLK_ASM)
 	putc (' ', print.outf);
 
       avoid_paste = false;
--- gcc/testsuite/g++.dg/warn/Wempty-body-1.C.jj	2008-11-10 11:46:08.000000000 +0100
+++ gcc/testsuite/g++.dg/warn/Wempty-body-1.C	2008-11-10 14:09:09.000000000 +0100
@@ -0,0 +1,39 @@
+// PR c++/36478
+// { dg-do compile }
+// { dg-options "-Wempty-body" }
+
+int
+main (int, char **)
+{
+#define NOPE
+  while (0);			// { dg-warning "suggest a space before " }
+#define M1 while (0);
+  M1				// { dg-warning "suggest a space before " }
+  while (0)/**/;
+  while (0)NOPE;
+#define M2 while (0)/**/;
+  M2
+#define M3 while (0)NOPE;
+  M3
+  while (0) ;
+  while (0)
+    ;
+  while (0)
+; 
+#define M4 while (0) ;
+  M4
+  while (0)\
+\
+;
+#define M5 while (0)\
+\
+;
+  M5				// { dg-warning "suggest a space before " }
+#define M6(a) while (0)
+#define M7 ;
+  M6(0)M7
+#define M8 M6(1)M7
+  M8
+}
+
+// { dg-warning "suggest a space before " "" { target *-*-* } 25 }
--- gcc/testsuite/g++.dg/warn/Wempty-body-2.C.jj	2008-11-10 11:47:05.000000000 +0100
+++ gcc/testsuite/g++.dg/warn/Wempty-body-2.C	2008-11-10 14:09:24.000000000 +0100
@@ -0,0 +1,40 @@
+// PR c++/36478
+// { dg-do compile }
+// { dg-options "-Wempty-body -save-temps" }
+
+int
+main (int, char **)
+{
+#define NOPE
+  while (0);			// { dg-warning "suggest a space before " }
+#define M1 while (0);
+  M1				// { dg-warning "suggest a space before " }
+  while (0)/**/;
+  while (0)NOPE;
+#define M2 while (0)/**/;
+  M2
+#define M3 while (0)NOPE;
+  M3
+  while (0) ;
+  while (0)
+    ;
+  while (0)
+; 
+#define M4 while (0) ;
+  M4
+  while (0)\
+\
+;
+#define M5 while (0)\
+\
+;
+  M5				// { dg-warning "suggest a space before " }
+#define M6(a) while (0)
+#define M7 ;
+  M6(0)M7
+#define M8 M6(1)M7
+  M8
+}
+
+// { dg-warning "suggest a space before " "" { target *-*-* } 25 }
+// { dg-final { cleanup-saved-temps } }

	Jakub

Re: [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)

[Mark Mitchell]

From a technical perspective, I think this patch makes sense.  In
particular, using a flag from the preprocessor, rather than poking at
location information, seems OK to me.

But, do we really want to warn about:

  while (0);

but not:

  while (0) ;

?

That seems surprising to me.

I can understand not warning about:

  while (0)
    ;

which seems a more explicit indication that the user has an infinite
loop there.

If we're using the single space as a way of making -save-temps work
(because an empty macro gets replaced with a space), I think that's just
as fragile a hack as we had before.  If we really need to make
-save-temps work, then we need -save-temps to emit something explicit to
mark the presence of the macro.

But, I also don't see it as a crisis if -save-temps doesn't work in this
particular case.  Once we decide that the presence of a macro is a
significant fact, we also accept that preprocessing the code is going to
result in a change.  That might argue that using the presence of a macro
is a mistake...

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)

[Jakub Jelinek]

On Mon, Nov 10, 2008 at 11:33:26AM -0800, Mark Mitchell wrote:
> >From a technical perspective, I think this patch makes sense.  In
> particular, using a flag from the preprocessor, rather than poking at
> location information, seems OK to me.
> 
> But, do we really want to warn about:
> 
>   while (0);
> 
> but not:
> 
>   while (0) ;
> 
> ?

A lot of people write for (a = b; something; somethingelse) ;
or for (a = b; something; somethingelse)/* Empty */;
if everything needed for the loop happens in the condition
or increment or both.  I don't think we want to add extra cases where
we want to warn, it will make the warning completely useless.

> That seems surprising to me.
> 
> I can understand not warning about:
> 
>   while (0)
>     ;

That's just one of the possible styles people use to mark an empty body
of a loop.  A space is another one, a comment another one, a macro another
one.  The alternative is IMHO just reverting Mike's 2007-05-07 changes.

> If we're using the single space as a way of making -save-temps work
> (because an empty macro gets replaced with a space), I think that's just
> as fragile a hack as we had before.  If we really need to make

Why?  Space is used by the preprocessor generally as a token separator
if whitespace is needed and we don't preserve columns with -E anyway.

> -save-temps work, then we need -save-temps to emit something explicit to
> mark the presence of the macro.
> 
> But, I also don't see it as a crisis if -save-temps doesn't work in this
> particular case.  Once we decide that the presence of a macro is a
> significant fact, we also accept that preprocessing the code is going to
> result in a change.  That might argue that using the presence of a macro
> is a mistake...

-save-temps is perhaps not 100% important, though many people will moan, but
ccache is important for a lot of users.

	Jakub

Re: [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)

[Mark Mitchell]

Jakub Jelinek wrote:

> A lot of people write for (a = b; something; somethingelse) ;
> or for (a = b; something; somethingelse)/* Empty */;

Yes, but I bet a lot of them also write:

 for (a = b; something; somethingelse);

The significance of that whitespace seems very questionable to me.

> -save-temps is perhaps not 100% important, though many people will moan, but
> ccache is important for a lot of users.

The problem is that I don't think the space is significant.  And, if
it's not, then replacing an macro that expands to nothing with a space
leaves the pre-processed source ambiguous: is the space an insignificant
space provided by the user or is it a significant space resulting from
macro expansion?  We can't tell.

Maybe this just argues for reverting Mike's patch.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)

[Jakub Jelinek]

On Tue, Nov 11, 2008 at 06:01:57PM -0800, Mark Mitchell wrote:
> Maybe this just argues for reverting Mike's patch.

Fine with my, should I prepare a patch?

	Jakub

Re: [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)

[Mark Mitchell]

Jakub Jelinek wrote:
> On Tue, Nov 11, 2008 at 06:01:57PM -0800, Mark Mitchell wrote:
>> Maybe this just argues for reverting Mike's patch.
> 
> Fine with my, should I prepare a patch?

Yes, I think that's probably the thing to do.

I think that Mike's patch is too closely tied to the coding conventions
of particular users.  If we're going to warn about this at all (and I
think we should), we should do so independent of the exact use of
whitespace and such.

I believe that Lint had special comments to disable certain warnings.
If we want, we could have comments/pragmas/etc. that users could use to
explicitly disable this warning.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

[C++ PATCH] Revert -Wempty-body C++ for/while warning (PR c++/36478)

[Jakub Jelinek]

On Wed, Nov 12, 2008 at 10:15:48AM -0800, Mark Mitchell wrote:
> Jakub Jelinek wrote:
> > Fine with my, should I prepare a patch?
> 
> Yes, I think that's probably the thing to do.
> 
> I think that Mike's patch is too closely tied to the coding conventions
> of particular users.  If we're going to warn about this at all (and I
> think we should), we should do so independent of the exact use of
> whitespace and such.
> 
> I believe that Lint had special comments to disable certain warnings.
> If we want, we could have comments/pragmas/etc. that users could use to
> explicitly disable this warning.

Here it is then, bootstrapped/regtested on x86_64-linux.  Ok for trunk?

2008-11-12  Jakub Jelinek  <jakub@redhat.com>

	PR c++/36478
	Revert:
	2007-05-07  Mike Stump  <mrs@apple.com>
	* doc/invoke.texi (Warning Options): Document that -Wempty-body
	also checks for and while statements in C++.

	Revert:
	2007-05-07  Mike Stump  <mrs@apple.com>
	* parser.c (check_empty_body): Add.
	(cp_parser_iteration_statement): Add call to check_empty_body.

	* g++.old-deja/g++.mike/empty.C: Remove.

--- gcc/cp/parser.c.jj	2008-11-12 13:33:02.000000000 +0100
+++ gcc/cp/parser.c	2008-11-12 20:38:34.000000000 +0100
@@ -7427,48 +7427,6 @@ 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
-      && close_loc.column+1 == semi_loc.column)
-    warning (OPT_Wempty_body,
-	     "suggest a space before %<;%> or explicit braces around empty "
-	     "body in %<%s%> statement",
-	     type);
-}
-
 /* Parse an iteration-statement.
 
    iteration-statement:
@@ -7511,7 +7469,6 @@ 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.  */
@@ -7573,7 +7530,6 @@ 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, "%<)%>");
 
--- gcc/doc/invoke.texi.jj	2008-10-23 09:07:05.000000000 +0200
+++ gcc/doc/invoke.texi	2008-11-12 20:39:30.000000000 +0100
@@ -3678,9 +3678,7 @@ integers are disabled by default in C++ 
 @opindex Wempty-body
 @opindex Wno-empty-body
 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}.
+while} statement.  This warning is also enabled by @option{-Wextra}.
 
 @item -Wenum-compare @r{(C++ and Objective-C++ only)}
 @opindex Wenum-compare
--- gcc/testsuite/g++.old-deja/g++.mike/empty.C.jj	2008-09-30 16:55:03.000000000 +0200
+++ gcc/testsuite/g++.old-deja/g++.mike/empty.C	2008-11-12 20:40:46.000000000 +0100
@@ -1,25 +0,0 @@
-// { dg-options "-W" }
-
-#define NOPE
-
-void foo() {
-  while (1); /* { dg-warning "suggest a space before " } */
-    {
-    }
-  for (;;); /* { dg-warning "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" } */
-  for (;;) NOPE; /* { dg-bogus "suggest a space before " "suggest" } */
-  while (1)
-    NOPE;
-  for (;;)
-    NOPE;
-}


	Jakub

Re: [C++ PATCH] Revert -Wempty-body C++ for/while warning (PR c++/36478)

[Mark Mitchell]

Jakub Jelinek wrote:

> 2008-11-12  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/36478
> 	Revert:
> 	2007-05-07  Mike Stump  <mrs@apple.com>
> 	* doc/invoke.texi (Warning Options): Document that -Wempty-body
> 	also checks for and while statements in C++.
> 
> 	Revert:
> 	2007-05-07  Mike Stump  <mrs@apple.com>
> 	* parser.c (check_empty_body): Add.
> 	(cp_parser_iteration_statement): Add call to check_empty_body.
> 
> 	* g++.old-deja/g++.mike/empty.C: Remove.

OK.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713