diff mbox

[C] Better diagnostics for C++ comments in C90 (PR c/61854)

Message ID 20140915210035.GH23226@redhat.com
State New
Headers show

Commit Message

Marek Polacek Sept. 15, 2014, 9 p.m. UTC 
On Mon, Sep 15, 2014 at 05:49:25PM +0000, Joseph S. Myers wrote:
> On Mon, 15 Sep 2014, Marek Polacek wrote:
> 
> > We must be careful to properly handle code such as "1 //**/ 2", which
> > has a different meaning in C90 and GNU90 mode.  New testcases test this.
> 
> I don't think there's sufficient allowance here for other valid cases.  
> It's valid to have // inside #if 0 in C90, for example, so that must not 
> be diagnosed (must not have a pedwarn or error, at least, that is).  It's 

Good point, sorry about that.  Luckily this can be fixed just by
checking pfile->state.skipping.  New test added.

> also valid to have it in a macro expansion; e.g.:
> 
> #define h(x) #x
> #define s(x) h(x)
> #define foo //
> 
> and then s(foo) must expand to the string "//".
> 
> Clearly, in any case, with or without the diagnostics, these cases should 
> have testcases in the testsuite.  But because // is only invalid in C90 if 
> it actually results in two consecutive / tokens (not just preprocessing 
> tokens), as such consecutive tokens are not part of any valid C90 program, 
> a more conservative approach may be needed to avoid errors for valid 
> cases.

I added a test for that and disabled the error for case we're in a
directive.  Thanks.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2014-09-15  Marek Polacek  <polacek@redhat.com>

	PR c/61854
libcpp/
	* init.c (struct lang_flags): Remove cplusplus_comments.
	(cpp_set_lang): Likewise.
	(post_options): Likewise.
	* lex.c (_cpp_lex_direct): Disallow C++ style comments in C90/C94.
testsuite/
	* gcc.dg/cpp/pr61854-1.c: New test.
	* gcc.dg/cpp/pr61854-2.c: New test.
	* gcc.dg/cpp/pr61854-3.c: New test.
	* gcc.dg/cpp/pr61854-3.h: New test.
	* gcc.dg/cpp/pr61854-4.c: New test.
	* gcc.dg/cpp/pr61854-5.c: New test.
	* gcc.dg/cpp/pr61854-c90.c: New test.
	* gcc.dg/cpp/pr61854-c94.c: New test.


	Marek

Comments

Joseph Myers Sept. 16, 2014, 10:32 p.m. UTC | #1 
On Mon, 15 Sep 2014, Marek Polacek wrote:

> On Mon, Sep 15, 2014 at 05:49:25PM +0000, Joseph S. Myers wrote:
> > On Mon, 15 Sep 2014, Marek Polacek wrote:
> > 
> > > We must be careful to properly handle code such as "1 //**/ 2", which
> > > has a different meaning in C90 and GNU90 mode.  New testcases test this.
> > 
> > I don't think there's sufficient allowance here for other valid cases.  
> > It's valid to have // inside #if 0 in C90, for example, so that must not 
> > be diagnosed (must not have a pedwarn or error, at least, that is).  It's 
> 
> Good point, sorry about that.  Luckily this can be fixed just by
> checking pfile->state.skipping.  New test added.

This is getting closer, but it looks like you still treat it as a line 
comment when being skipped for C90, when actually it's not safe to treat 
it like that; you have to produce a '/' preprocessing token and continue 
tokenizing the rest of the line.  Consider the following code:

int i = 0
#if 0
// /*
#else
// */
+1
#endif
;

For C90 i gets value 0.  With // comments it gets value 1.

> +	  /* In C89/C94, C++ style comments are forbidden.  */
> +	  else if ((CPP_OPTION (pfile, lang) == CLK_STDC89
> +		    || CPP_OPTION (pfile, lang) == CLK_STDC94))
> +	    {
> +	      /* But don't be confused about // immediately followed by *.  */
> +	      if (buffer->cur[1] == '*'
> +		  || pfile->state.in_directive)

And this comment needs updating to reflect that it's not just //* where // 
can appear in valid C90 code in a way incompatible with treating it as a 
comment.
diff mbox

Patch

diff --git gcc/gcc/testsuite/gcc.dg/cpp/pr61854-1.c gcc/gcc/testsuite/gcc.dg/cpp/pr61854-1.c
index e69de29..364a511 100644
--- gcc/gcc/testsuite/gcc.dg/cpp/pr61854-1.c
+++ gcc/gcc/testsuite/gcc.dg/cpp/pr61854-1.c
@@ -0,0 +1,15 @@ 
+/* PR c/61854 */
+/* { dg-do run } */
+/* { dg-options "-std=c89" } */
+
+int
+main (void)
+{
+  int i = 1 //**/ 2
+  ;
+  int j = 1 //**/ 2
+  ;
+  if (i != 0 || j != 0)
+    __builtin_abort ();  
+  return 0;
+}
diff --git gcc/gcc/testsuite/gcc.dg/cpp/pr61854-2.c gcc/gcc/testsuite/gcc.dg/cpp/pr61854-2.c
index e69de29..883db21 100644
--- gcc/gcc/testsuite/gcc.dg/cpp/pr61854-2.c
+++ gcc/gcc/testsuite/gcc.dg/cpp/pr61854-2.c
@@ -0,0 +1,15 @@ 
+/* PR c/61854 */
+/* { dg-do run } */
+/* { dg-options "-std=gnu89" } */
+
+int
+main (void)
+{
+  int i = 1 //**/ 2
+  ;
+  int j = 1 //**/ 2
+  ;
+  if (i != 1 || j != 1)
+    __builtin_abort ();  
+  return 0;
+}
diff --git gcc/gcc/testsuite/gcc.dg/cpp/pr61854-3.c gcc/gcc/testsuite/gcc.dg/cpp/pr61854-3.c
index e69de29..916c12e 100644
--- gcc/gcc/testsuite/gcc.dg/cpp/pr61854-3.c
+++ gcc/gcc/testsuite/gcc.dg/cpp/pr61854-3.c
@@ -0,0 +1,6 @@ 
+/* PR c/61854 */
+/* { dg-do preprocess } */
+/* { dg-options "-std=c89" } */
+
+#include "pr61854-3.h"
+int i;
diff --git gcc/gcc/testsuite/gcc.dg/cpp/pr61854-3.h gcc/gcc/testsuite/gcc.dg/cpp/pr61854-3.h
index e69de29..fd798bd 100644
--- gcc/gcc/testsuite/gcc.dg/cpp/pr61854-3.h
+++ gcc/gcc/testsuite/gcc.dg/cpp/pr61854-3.h
@@ -0,0 +1,4 @@ 
+#pragma GCC system_header
+// X
+// Y
+// Z
diff --git gcc/gcc/testsuite/gcc.dg/cpp/pr61854-4.c gcc/gcc/testsuite/gcc.dg/cpp/pr61854-4.c
index e69de29..25cc4b2 100644
--- gcc/gcc/testsuite/gcc.dg/cpp/pr61854-4.c
+++ gcc/gcc/testsuite/gcc.dg/cpp/pr61854-4.c
@@ -0,0 +1,16 @@ 
+/* PR c/61854 */
+/* { dg-do preprocess } */
+/* { dg-options "-std=c89" } */
+
+void
+foo (void)
+{
+#if 0
+  // Don't error here.
+#endif
+#if 1
+  // But error here.
+#endif
+  /* { dg-error "C\\+\\+ style comments are not allowed in ISO C90" "comments"  { target *-*-*} 12 } */
+  /* { dg-error "reported only once" ""  { target *-*-*} 12 } */
+}
diff --git gcc/gcc/testsuite/gcc.dg/cpp/pr61854-5.c gcc/gcc/testsuite/gcc.dg/cpp/pr61854-5.c
index e69de29..a7628dc 100644
--- gcc/gcc/testsuite/gcc.dg/cpp/pr61854-5.c
+++ gcc/gcc/testsuite/gcc.dg/cpp/pr61854-5.c
@@ -0,0 +1,15 @@ 
+/* PR c/61854 */
+/* { dg-do run } */
+/* { dg-options "-std=c89" } */
+
+#define h(x) #x
+#define s(x) h(x)
+#define foo //
+
+int
+main (void)
+{
+  if (__builtin_memcmp (s(foo), "//", 3) != 0)
+    __builtin_abort ();
+  return 0;
+}
diff --git gcc/gcc/testsuite/gcc.dg/cpp/pr61854-c90.c gcc/gcc/testsuite/gcc.dg/cpp/pr61854-c90.c
index e69de29..37eecbe 100644
--- gcc/gcc/testsuite/gcc.dg/cpp/pr61854-c90.c
+++ gcc/gcc/testsuite/gcc.dg/cpp/pr61854-c90.c
@@ -0,0 +1,13 @@ 
+/* PR c/61854 */
+/* { dg-do preprocess } */
+/* { dg-options "-std=iso9899:1990" } */
+
+void
+foo (void)
+{
+  // 1st
+  /* { dg-error "C\\+\\+ style comments are not allowed in ISO C90" "comments"  { target *-*-*} 8 } */
+  /* { dg-error "reported only once" ""  { target *-*-*} 8 } */
+  // 2nd
+  // 3rd
+}
diff --git gcc/gcc/testsuite/gcc.dg/cpp/pr61854-c94.c gcc/gcc/testsuite/gcc.dg/cpp/pr61854-c94.c
index e69de29..64f1e18 100644
--- gcc/gcc/testsuite/gcc.dg/cpp/pr61854-c94.c
+++ gcc/gcc/testsuite/gcc.dg/cpp/pr61854-c94.c
@@ -0,0 +1,13 @@ 
+/* PR c/61854 */
+/* { dg-do preprocess } */
+/* { dg-options "-std=iso9899:199409" } */
+
+void
+foo (void)
+{
+  // 1st
+  /* { dg-error "C\\+\\+ style comments are not allowed in ISO C90" "comments"  { target *-*-*} 8 } */
+  /* { dg-error "reported only once" ""  { target *-*-*} 8 } */
+  // 2nd
+  // 3rd
+}
diff --git gcc/libcpp/init.c gcc/libcpp/init.c
index d612374..1121962 100644
--- gcc/libcpp/init.c
+++ gcc/libcpp/init.c
@@ -83,7 +83,6 @@  struct lang_flags
   char extended_identifiers;
   char c11_identifiers;
   char std;
-  char cplusplus_comments;
   char digraphs;
   char uliterals;
   char rliterals;
@@ -94,23 +93,23 @@  struct lang_flags
 };
 
 static const struct lang_flags lang_defaults[] =
-{ /*              c99 c++ xnum xid c11 std // digr ulit rlit udlit bincst digsep trig */
-  /* GNUC89   */  { 0,  0,  1,  0,  0,  0,  1,  1,  0,   0,   0,    0,     0,     0 },
-  /* GNUC99   */  { 1,  0,  1,  0,  0,  0,  1,  1,  1,   1,   0,    0,     0,     0 },
-  /* GNUC11   */  { 1,  0,  1,  0,  1,  0,  1,  1,  1,   1,   0,    0,     0,     0 },
-  /* STDC89   */  { 0,  0,  0,  0,  0,  1,  0,  0,  0,   0,   0,    0,     0,     1 },
-  /* STDC94   */  { 0,  0,  0,  0,  0,  1,  0,  1,  0,   0,   0,    0,     0,     1 },
-  /* STDC99   */  { 1,  0,  1,  0,  0,  1,  1,  1,  0,   0,   0,    0,     0,     1 },
-  /* STDC11   */  { 1,  0,  1,  0,  1,  1,  1,  1,  1,   0,   0,    0,     0,     1 },
-  /* GNUCXX   */  { 0,  1,  1,  0,  0,  0,  1,  1,  0,   0,   0,    0,     0,     0 },
-  /* CXX98    */  { 0,  1,  0,  0,  0,  1,  1,  1,  0,   0,   0,    0,     0,     1 },
-  /* GNUCXX11 */  { 1,  1,  1,  0,  1,  0,  1,  1,  1,   1,   1,    0,     0,     0 },
-  /* CXX11    */  { 1,  1,  1,  0,  1,  1,  1,  1,  1,   1,   1,    0,     0,     1 },
-  /* GNUCXX14 */  { 1,  1,  1,  0,  1,  0,  1,  1,  1,   1,   1,    1,     1,     0 },
-  /* CXX14    */  { 1,  1,  1,  0,  1,  1,  1,  1,  1,   1,   1,    1,     1,     1 },
-  /* GNUCXX1Z */  { 1,  1,  1,  0,  1,  0,  1,  1,  1,   1,   1,    1,     1,     0 },
-  /* CXX1Z    */  { 1,  1,  1,  0,  1,  1,  1,  1,  1,   1,   1,    1,     1,     0 },
-  /* ASM      */  { 0,  0,  1,  0,  0,  0,  1,  0,  0,   0,   0,    0,     0,     0 }
+{ /*              c99 c++ xnum xid c11 std digr ulit rlit udlit bincst digsep trig */
+  /* GNUC89   */  { 0,  0,  1,  0,  0,  0,  1,   0,   0,   0,    0,     0,     0 },
+  /* GNUC99   */  { 1,  0,  1,  0,  0,  0,  1,   1,   1,   0,    0,     0,     0 },
+  /* GNUC11   */  { 1,  0,  1,  0,  1,  0,  1,   1,   1,   0,    0,     0,     0 },
+  /* STDC89   */  { 0,  0,  0,  0,  0,  1,  0,   0,   0,   0,    0,     0,     1 },
+  /* STDC94   */  { 0,  0,  0,  0,  0,  1,  1,   0,   0,   0,    0,     0,     1 },
+  /* STDC99   */  { 1,  0,  1,  0,  0,  1,  1,   0,   0,   0,    0,     0,     1 },
+  /* STDC11   */  { 1,  0,  1,  0,  1,  1,  1,   1,   0,   0,    0,     0,     1 },
+  /* GNUCXX   */  { 0,  1,  1,  0,  0,  0,  1,   0,   0,   0,    0,     0,     0 },
+  /* CXX98    */  { 0,  1,  0,  0,  0,  1,  1,   0,   0,   0,    0,     0,     1 },
+  /* GNUCXX11 */  { 1,  1,  1,  0,  1,  0,  1,   1,   1,   1,    0,     0,     0 },
+  /* CXX11    */  { 1,  1,  1,  0,  1,  1,  1,   1,   1,   1,    0,     0,     1 },
+  /* GNUCXX14 */  { 1,  1,  1,  0,  1,  0,  1,   1,   1,   1,    1,     1,     0 },
+  /* CXX14    */  { 1,  1,  1,  0,  1,  1,  1,   1,   1,   1,    1,     1,     1 },
+  /* GNUCXX1Z */  { 1,  1,  1,  0,  1,  0,  1,   1,   1,   1,    1,     1,     0 },
+  /* CXX1Z    */  { 1,  1,  1,  0,  1,  1,  1,   1,   1,   1,    1,     1,     0 },
+  /* ASM      */  { 0,  0,  1,  0,  0,  0,  0,   0,   0,   0,    0,     0,     0 }
   /* xid should be 1 for GNUC99, STDC99, GNUCXX, CXX98, GNUCXX11, CXX11,
      GNUCXX14, and CXX14 when no longer experimental (when all uses of
      identifiers in the compiler have been audited for correct handling
@@ -131,7 +130,6 @@  cpp_set_lang (cpp_reader *pfile, enum c_lang lang)
   CPP_OPTION (pfile, extended_identifiers)	 = l->extended_identifiers;
   CPP_OPTION (pfile, c11_identifiers)		 = l->c11_identifiers;
   CPP_OPTION (pfile, std)			 = l->std;
-  CPP_OPTION (pfile, cplusplus_comments)	 = l->cplusplus_comments;
   CPP_OPTION (pfile, digraphs)			 = l->digraphs;
   CPP_OPTION (pfile, uliterals)			 = l->uliterals;
   CPP_OPTION (pfile, rliterals)			 = l->rliterals;
@@ -775,8 +773,6 @@  post_options (cpp_reader *pfile)
 
   if (CPP_OPTION (pfile, traditional))
     {
-      CPP_OPTION (pfile, cplusplus_comments) = 0;
-
       CPP_OPTION (pfile, trigraphs) = 0;
       CPP_OPTION (pfile, warn_trigraphs) = 0;
     }
diff --git gcc/libcpp/lex.c gcc/libcpp/lex.c
index 5366dad..13b8ce5 100644
--- gcc/libcpp/lex.c
+++ gcc/libcpp/lex.c
@@ -2322,13 +2322,16 @@  _cpp_lex_direct (cpp_reader *pfile)
 	  if (_cpp_skip_block_comment (pfile))
 	    cpp_error (pfile, CPP_DL_ERROR, "unterminated comment");
 	}
-      else if (c == '/' && (CPP_OPTION (pfile, cplusplus_comments)
-			    || cpp_in_system_header (pfile)))
+      else if (c == '/' && ! CPP_OPTION (pfile, traditional))
 	{
+	  /* Don't warn for system headers.  */
+	  if (cpp_in_system_header (pfile))
+	    ;
 	  /* Warn about comments if pedantically GNUC89, and not
 	     in system headers.  */
-	  if (CPP_OPTION (pfile, lang) == CLK_GNUC89 && CPP_PEDANTIC (pfile)
-	      && ! buffer->warned_cplusplus_comments)
+	  else if (CPP_OPTION (pfile, lang) == CLK_GNUC89
+		   && CPP_PEDANTIC (pfile)
+		   && ! buffer->warned_cplusplus_comments)
 	    {
 	      cpp_error (pfile, CPP_DL_PEDWARN,
 			 "C++ style comments are not allowed in ISO C90");
@@ -2347,7 +2350,28 @@  _cpp_lex_direct (cpp_reader *pfile)
 			 "(this will be reported only once per input file)");
 	      buffer->warned_cplusplus_comments = 1;
 	    }
-
+	  /* In C89/C94, C++ style comments are forbidden.  */
+	  else if ((CPP_OPTION (pfile, lang) == CLK_STDC89
+		    || CPP_OPTION (pfile, lang) == CLK_STDC94))
+	    {
+	      /* But don't be confused about // immediately followed by *.  */
+	      if (buffer->cur[1] == '*'
+		  || pfile->state.in_directive)
+		{
+		  result->type = CPP_DIV;
+		  break;
+		}
+	      else if (! buffer->warned_cplusplus_comments
+		       && ! pfile->state.skipping)
+		{
+		  cpp_error (pfile, CPP_DL_ERROR,
+			     "C++ style comments are not allowed in ISO C90");
+		  cpp_error (pfile, CPP_DL_ERROR,
+			     "(this will be reported only once per input "
+			     "file)");
+		  buffer->warned_cplusplus_comments = 1;
+		}
+	    }
 	  if (skip_line_comment (pfile) && CPP_OPTION (pfile, warn_comments))
 	    cpp_warning (pfile, CPP_W_COMMENTS, "multi-line comment");
 	}