* [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)
@ 2008-11-10 16:09 Jakub Jelinek
2008-11-10 19:36 ` Mark Mitchell
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2008-11-10 16:09 UTC (permalink / raw)
To: Tom Tromey, Jason Merrill, Mark Mitchell; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)
2008-11-10 16:09 [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478) Jakub Jelinek
@ 2008-11-10 19:36 ` Mark Mitchell
2008-11-10 20:10 ` Jakub Jelinek
0 siblings, 1 reply; 8+ messages in thread
From: Mark Mitchell @ 2008-11-10 19:36 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Tom Tromey, Jason Merrill, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)
2008-11-10 19:36 ` Mark Mitchell
@ 2008-11-10 20:10 ` Jakub Jelinek
2008-11-12 4:18 ` Mark Mitchell
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2008-11-10 20:10 UTC (permalink / raw)
To: Mark Mitchell; +Cc: Tom Tromey, Jason Merrill, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)
2008-11-10 20:10 ` Jakub Jelinek
@ 2008-11-12 4:18 ` Mark Mitchell
2008-11-12 12:53 ` Jakub Jelinek
0 siblings, 1 reply; 8+ messages in thread
From: Mark Mitchell @ 2008-11-12 4:18 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Tom Tromey, Jason Merrill, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)
2008-11-12 4:18 ` Mark Mitchell
@ 2008-11-12 12:53 ` Jakub Jelinek
2008-11-12 18:58 ` Mark Mitchell
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2008-11-12 12:53 UTC (permalink / raw)
To: Mark Mitchell; +Cc: Tom Tromey, Jason Merrill, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478)
2008-11-12 12:53 ` Jakub Jelinek
@ 2008-11-12 18:58 ` Mark Mitchell
2008-11-12 21:24 ` [C++ PATCH] Revert -Wempty-body C++ for/while " Jakub Jelinek
0 siblings, 1 reply; 8+ messages in thread
From: Mark Mitchell @ 2008-11-12 18:58 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Tom Tromey, Jason Merrill, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [C++ PATCH] Revert -Wempty-body C++ for/while warning (PR c++/36478)
2008-11-12 18:58 ` Mark Mitchell
@ 2008-11-12 21:24 ` Jakub Jelinek
2008-11-13 0:09 ` Mark Mitchell
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2008-11-12 21:24 UTC (permalink / raw)
To: Mark Mitchell; +Cc: Tom Tromey, Jason Merrill, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [C++ PATCH] Revert -Wempty-body C++ for/while warning (PR c++/36478)
2008-11-12 21:24 ` [C++ PATCH] Revert -Wempty-body C++ for/while " Jakub Jelinek
@ 2008-11-13 0:09 ` Mark Mitchell
0 siblings, 0 replies; 8+ messages in thread
From: Mark Mitchell @ 2008-11-13 0:09 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Tom Tromey, Jason Merrill, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-11-12 22:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-11-10 16:09 [libcpp+C++ PATCH] Fix -Wempty-body C++ warning (PR c++/36478) Jakub Jelinek
2008-11-10 19:36 ` Mark Mitchell
2008-11-10 20:10 ` Jakub Jelinek
2008-11-12 4:18 ` Mark Mitchell
2008-11-12 12:53 ` Jakub Jelinek
2008-11-12 18:58 ` Mark Mitchell
2008-11-12 21:24 ` [C++ PATCH] Revert -Wempty-body C++ for/while " Jakub Jelinek
2008-11-13 0:09 ` Mark Mitchell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).