C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Marek Polacek]

Spurred by the recent <http://www.viva64.com/en/b/0425/> findings, I decided to
implement a warning that warns when a pointer is compared with a zero character
literal (constant), because this isn't likely to be intended.  So e.g.

  ptr == L'\0'

is probably wrong and should've been written as

  ptr[0] == L'\0'

Jonathan pointed out that this would actually be invalid C++11 since pointer
conversions are only allowed for integer literals, not char literals.
There's -Wzero-as-null-pointer-constant to catch other cases such as

  void *o = '\0';

This patch deals strictly with ==/!=.

Now, I'm not exactly sure how to name this, -Wpointer-compare seems pretty
general.  So maybe as a followup I could use this option for C's
"comparison between pointer and integer" (that's a pedwarn)?

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

2016-09-10  Marek Polacek  <polacek@redhat.com>

	PR c++/64767
	* c.opt (Wpointer-compare): New option.

	* c-parser.c (c_parser_postfix_expression): Mark zero character
	constants by setting original_type in c_expr.
	* c-typeck.c (parser_build_binary_op): Warn when a pointer is compared
	with a zero character constant.

	* cp-tree.h (class cp_expr): Add char_literal_p member.  Update the
	constructors and the copy constructor.
	* parser.c (cp_parser_primary_expression): Mark zero character
	literals.
	(cp_parser_binary_expression): Warn when a pointer is compared with a
	zero character literal.

	* doc/invoke.texi: Document -Wpointer-compare.

	* c-c++-common/Wpointer-compare-1.c: New test.

diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index a5358ed..ac299b6 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -776,6 +776,10 @@ Wpointer-sign
 C ObjC Var(warn_pointer_sign) Warning LangEnabledBy(C ObjC,Wall || Wpedantic)
 Warn when a pointer differs in signedness in an assignment.
 
+Wpointer-compare
+C ObjC C++ ObjC++ Var(warn_pointer_compare) Init(1) Warning
+Warn when a pointer is compared with a zero character constant.
+
 Wpointer-to-int-cast
 C ObjC Var(warn_pointer_to_int_cast) Init(1) Warning
 Warn when a pointer is cast to an integer of a different size.
diff --git gcc/c/c-parser.c gcc/c/c-parser.c
index 0aba51c..46ca08a 100644
--- gcc/c/c-parser.c
+++ gcc/c/c-parser.c
@@ -7520,6 +7520,9 @@ c_parser_postfix_expression (c_parser *parser)
     case CPP_CHAR32:
     case CPP_WCHAR:
       expr.value = c_parser_peek_token (parser)->value;
+      /* For the purpose of warning when a pointer is compared with
+	 a zero character constant.  */
+      expr.original_type = char_type_node;
       set_c_expr_source_range (&expr, tok_range);
       c_parser_consume_token (parser);
       break;
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index d56c3d6..6426ae6 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3709,6 +3709,21 @@ parser_build_binary_op (location_t location, enum tree_code code,
 	      && !integer_zerop (tree_strip_nop_conversions (arg1.value))))
 	warning_at (location, OPT_Waddress,
 		    "comparison with string literal results in unspecified behavior");
+      /* Warn for ptr == '\0', it's likely that it should've been ptr[0].  */
+      if (POINTER_TYPE_P (type1)
+	   && null_pointer_constant_p (arg2.value)
+	   && arg2.original_type == char_type_node
+	   && warning_at (location, OPT_Wpointer_compare,
+			  "comparison between pointer and zero character "
+			  "constant"))
+	inform (arg1.get_start (), "did you mean to dereference the pointer?");
+      else if (POINTER_TYPE_P (type2)
+	       && null_pointer_constant_p (arg1.value)
+	       && arg1.original_type == char_type_node
+	       && warning_at (location, OPT_Wpointer_compare,
+			      "comparison between pointer and zero character "
+			      "constant"))
+	inform (arg2.get_start (), "did you mean to dereference the pointer?");
     }
   else if (TREE_CODE_CLASS (code) == tcc_comparison
 	   && (code1 == STRING_CST || code2 == STRING_CST))
diff --git gcc/cp/cp-tree.h gcc/cp/cp-tree.h
index 5bcb98b..a29905b 100644
--- gcc/cp/cp-tree.h
+++ gcc/cp/cp-tree.h
@@ -51,16 +51,21 @@ class cp_expr
 {
 public:
   cp_expr () :
-    m_value (NULL), m_loc (UNKNOWN_LOCATION) {}
+    m_value (NULL), m_loc (UNKNOWN_LOCATION), m_char_literal_p (false) {}
 
   cp_expr (tree value) :
-    m_value (value), m_loc (EXPR_LOCATION (m_value)) {}
+    m_value (value), m_loc (EXPR_LOCATION (m_value)),
+    m_char_literal_p (false) {}
 
   cp_expr (tree value, location_t loc):
-    m_value (value), m_loc (loc) {}
+    m_value (value), m_loc (loc), m_char_literal_p (false) {}
+
+  cp_expr (tree value, location_t loc, bool char_literal_p):
+    m_value (value), m_loc (loc), m_char_literal_p (char_literal_p) {}
 
   cp_expr (const cp_expr &other) :
-    m_value (other.m_value), m_loc (other.m_loc) {}
+    m_value (other.m_value), m_loc (other.m_loc),
+    m_char_literal_p (other.m_char_literal_p) {}
 
   /* Implicit conversions to tree.  */
   operator tree () const { return m_value; }
@@ -91,9 +96,12 @@ public:
     set_location (make_location (m_loc, start, finish));
   }
 
+  bool is_char_literal_p () const { return m_char_literal_p; }
+
  private:
   tree m_value;
   location_t m_loc;
+  bool m_char_literal_p;
 };
 
 inline bool
diff --git gcc/cp/parser.c gcc/cp/parser.c
index ca9f8b9..fc916a0 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -4756,6 +4756,7 @@ cp_parser_primary_expression (cp_parser *parser,
 			      cp_id_kind *idk)
 {
   cp_token *token = NULL;
+  bool char_literal_p = false;
 
   /* Assume the primary expression is not an id-expression.  */
   *idk = CP_ID_KIND_NONE;
@@ -4777,6 +4778,7 @@ cp_parser_primary_expression (cp_parser *parser,
     case CPP_CHAR32:
     case CPP_WCHAR:
     case CPP_UTF8CHAR:
+      char_literal_p = true;
     case CPP_NUMBER:
     case CPP_PREPARSED_EXPR:
       if (TREE_CODE (token->u.value) == USERDEF_LITERAL)
@@ -4832,7 +4834,7 @@ cp_parser_primary_expression (cp_parser *parser,
 	  if (!cast_p)
 	    cp_parser_non_integral_constant_expression (parser, NIC_FLOAT);
 	}
-      return cp_expr (token->u.value, token->location);
+      return cp_expr (token->u.value, token->location, char_literal_p);
 
     case CPP_CHAR_USERDEF:
     case CPP_CHAR16_USERDEF:
@@ -8924,6 +8926,31 @@ cp_parser_binary_expression (cp_parser* parser, bool cast_p,
 	warn_logical_not_parentheses (current.loc, current.tree_type,
 				      current.lhs, maybe_constant_value (rhs));
 
+      /* Warn for ptr == '\0', it's likely that it should've been ptr[0].  */
+      if (current.tree_type == EQ_EXPR || current.tree_type == NE_EXPR)
+	{
+	  if (TREE_TYPE (current.lhs.get_value ())
+	      && POINTER_TYPE_P (TREE_TYPE (current.lhs.get_value ()))
+	      && integer_zerop (rhs.get_value ())
+	      && !TREE_OVERFLOW (rhs.get_value ())
+	      && rhs.is_char_literal_p ()
+	      && warning_at (current.loc, OPT_Wpointer_compare,
+			     "comparison between pointer and zero character "
+			     "literal"))
+	    inform (current.lhs.get_location (),
+		    "did you mean to dereference the pointer?");
+	  else if (TREE_TYPE (rhs.get_value ())
+		   && POINTER_TYPE_P (TREE_TYPE (rhs.get_value ()))
+		   && integer_zerop (current.lhs.get_value ())
+		   && !TREE_OVERFLOW (current.lhs.get_value ())
+		   && current.lhs.is_char_literal_p ()
+		   && warning_at (current.loc, OPT_Wpointer_compare,
+				  "comparison between pointer and zero "
+				  "character literal"))
+	    inform (rhs.get_location (),
+		    "did you mean to dereference the pointer?");
+
+	}
       overload = NULL;
 
       location_t combined_loc = make_location (current.loc,
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 20be9b7..cc19874 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -287,7 +287,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wpacked  -Wpacked-bitfield-compat  -Wpadded @gol
 -Wparentheses -Wno-pedantic-ms-format @gol
 -Wplacement-new -Wplacement-new=@var{n} @gol
--Wpointer-arith  -Wno-pointer-to-int-cast @gol
+-Wpointer-arith  -Wpointer-compare  -Wno-pointer-to-int-cast @gol
 -Wno-pragmas -Wredundant-decls  -Wno-return-local-addr @gol
 -Wreturn-type  -Wsequence-point  -Wshadow  -Wno-shadow-ivar @gol
 -Wshift-overflow -Wshift-overflow=@var{n} @gol
@@ -5115,6 +5115,20 @@ convenience in calculations with @code{void *} pointers and pointers
 to functions.  In C++, warn also when an arithmetic operation involves
 @code{NULL}.  This warning is also enabled by @option{-Wpedantic}.
 
+@item -Wpointer-compare
+@opindex Wpointer-compare
+@opindex Wno-pointer-compare
+Warn if a pointer is compared with a zero character constant.  This usually
+means that the pointer was meant to be dereferenced.  For example:
+
+@smallexample
+const char *p = foo ();
+if (p == '\0')
+  return 42;
+@end smallexample
+
+This warning is enabled by default.
+
 @item -Wtype-limits
 @opindex Wtype-limits
 @opindex Wno-type-limits
diff --git gcc/testsuite/c-c++-common/Wpointer-compare-1.c gcc/testsuite/c-c++-common/Wpointer-compare-1.c
index e69de29..80da85a 100644
--- gcc/testsuite/c-c++-common/Wpointer-compare-1.c
+++ gcc/testsuite/c-c++-common/Wpointer-compare-1.c
@@ -0,0 +1,81 @@
+/* PR c++/64767 */
+/* { dg-do compile } */
+/* { dg-options "-Wpointer-compare" } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+
+int
+f1 (int *p, int **q)
+{
+  int r = 0;
+
+  r += p == '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p == L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p == u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p == U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  r += '\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += '\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  r += q == '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q == L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q == u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q == U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  r += '\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += '\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  return r;
+}
+
+int
+f2 (int *p)
+{
+  int r = 0;
+
+  /* Keep quiet.  */
+  r += p == (void *) 0;
+  r += p != (void *) 0;
+  r += (void *) 0 == p;
+  r += (void *) 0 != p;
+
+  r += p == 0;
+  r += p != 0;
+  r += 0 == p;
+  r += 0 != p;
+
+  return r;
+}
+
+int
+f3 (int *p)
+{
+  int r = 0;
+
+  r += p == (char) 0;
+  r += p != (char) 0;
+
+  r += (char) 0 == p;
+  r += (char) 0 != p;
+
+  return r;
+}

	Marek

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Jakub Jelinek]

Hi!

Just a minor nit:

On Sat, Sep 10, 2016 at 04:58:43PM +0200, Marek Polacek wrote:
> Spurred by the recent <http://www.viva64.com/en/b/0425/> findings, I decided to
> implement a warning that warns when a pointer is compared with a zero character
> literal (constant), because this isn't likely to be intended.  So e.g.

> @@ -4777,6 +4778,7 @@ cp_parser_primary_expression (cp_parser *parser,
>      case CPP_CHAR32:
>      case CPP_WCHAR:
>      case CPP_UTF8CHAR:
> +      char_literal_p = true;
>      case CPP_NUMBER:
>      case CPP_PREPARSED_EXPR:
>        if (TREE_CODE (token->u.value) == USERDEF_LITERAL)

No /* FALLTHRU */ ?

	Jakub

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Marek Polacek]

On Sat, Sep 10, 2016 at 05:07:51PM +0200, Jakub Jelinek wrote:
> Hi!
> 
> Just a minor nit:
> 
> On Sat, Sep 10, 2016 at 04:58:43PM +0200, Marek Polacek wrote:
> > Spurred by the recent <http://www.viva64.com/en/b/0425/> findings, I decided to
> > implement a warning that warns when a pointer is compared with a zero character
> > literal (constant), because this isn't likely to be intended.  So e.g.
> 
> > @@ -4777,6 +4778,7 @@ cp_parser_primary_expression (cp_parser *parser,
> >      case CPP_CHAR32:
> >      case CPP_WCHAR:
> >      case CPP_UTF8CHAR:
> > +      char_literal_p = true;
> >      case CPP_NUMBER:
> >      case CPP_PREPARSED_EXPR:
> >        if (TREE_CODE (token->u.value) == USERDEF_LITERAL)
> 
> No /* FALLTHRU */ ?

Oh yeah, I'd completely forgotten.  Didn't mean to make the guy implementing
-Wimplicit-fallthrough sad...

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

2016-09-10  Marek Polacek  <polacek@redhat.com>

	PR c++/64767
	* c.opt (Wpointer-compare): New option.

	* c-parser.c (c_parser_postfix_expression): Mark zero character
	constants by setting original_type in c_expr.
	* c-typeck.c (parser_build_binary_op): Warn when a pointer is compared
	with a zero character constant.

	* cp-tree.h (class cp_expr): Add char_literal_p member.  Update the
	constructors and the copy constructor.
	* parser.c (cp_parser_primary_expression): Mark zero character
	literals.
	(cp_parser_binary_expression): Warn when a pointer is compared with a
	zero character literal.

	* doc/invoke.texi: Document -Wpointer-compare.

	* c-c++-common/Wpointer-compare-1.c: New test.

diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index c55c7c3..a93c07e 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -796,6 +796,10 @@ Wpointer-sign
 C ObjC Var(warn_pointer_sign) Warning LangEnabledBy(C ObjC,Wall || Wpedantic)
 Warn when a pointer differs in signedness in an assignment.
 
+Wpointer-compare
+C ObjC C++ ObjC++ Var(warn_pointer_compare) Init(1) Warning
+Warn when a pointer is compared with a zero character constant.
+
 Wpointer-to-int-cast
 C ObjC Var(warn_pointer_to_int_cast) Init(1) Warning
 Warn when a pointer is cast to an integer of a different size.
diff --git gcc/c/c-parser.c gcc/c/c-parser.c
index 0aba51c..46ca08a 100644
--- gcc/c/c-parser.c
+++ gcc/c/c-parser.c
@@ -7520,6 +7520,9 @@ c_parser_postfix_expression (c_parser *parser)
     case CPP_CHAR32:
     case CPP_WCHAR:
       expr.value = c_parser_peek_token (parser)->value;
+      /* For the purpose of warning when a pointer is compared with
+	 a zero character constant.  */
+      expr.original_type = char_type_node;
       set_c_expr_source_range (&expr, tok_range);
       c_parser_consume_token (parser);
       break;
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index d56c3d6..6426ae6 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3709,6 +3709,21 @@ parser_build_binary_op (location_t location, enum tree_code code,
 	      && !integer_zerop (tree_strip_nop_conversions (arg1.value))))
 	warning_at (location, OPT_Waddress,
 		    "comparison with string literal results in unspecified behavior");
+      /* Warn for ptr == '\0', it's likely that it should've been ptr[0].  */
+      if (POINTER_TYPE_P (type1)
+	   && null_pointer_constant_p (arg2.value)
+	   && arg2.original_type == char_type_node
+	   && warning_at (location, OPT_Wpointer_compare,
+			  "comparison between pointer and zero character "
+			  "constant"))
+	inform (arg1.get_start (), "did you mean to dereference the pointer?");
+      else if (POINTER_TYPE_P (type2)
+	       && null_pointer_constant_p (arg1.value)
+	       && arg1.original_type == char_type_node
+	       && warning_at (location, OPT_Wpointer_compare,
+			      "comparison between pointer and zero character "
+			      "constant"))
+	inform (arg2.get_start (), "did you mean to dereference the pointer?");
     }
   else if (TREE_CODE_CLASS (code) == tcc_comparison
 	   && (code1 == STRING_CST || code2 == STRING_CST))
diff --git gcc/cp/cp-tree.h gcc/cp/cp-tree.h
index d4bfb26..218c5a7 100644
--- gcc/cp/cp-tree.h
+++ gcc/cp/cp-tree.h
@@ -51,16 +51,21 @@ class cp_expr
 {
 public:
   cp_expr () :
-    m_value (NULL), m_loc (UNKNOWN_LOCATION) {}
+    m_value (NULL), m_loc (UNKNOWN_LOCATION), m_char_literal_p (false) {}
 
   cp_expr (tree value) :
-    m_value (value), m_loc (EXPR_LOCATION (m_value)) {}
+    m_value (value), m_loc (EXPR_LOCATION (m_value)),
+    m_char_literal_p (false) {}
 
   cp_expr (tree value, location_t loc):
-    m_value (value), m_loc (loc) {}
+    m_value (value), m_loc (loc), m_char_literal_p (false) {}
+
+  cp_expr (tree value, location_t loc, bool char_literal_p):
+    m_value (value), m_loc (loc), m_char_literal_p (char_literal_p) {}
 
   cp_expr (const cp_expr &other) :
-    m_value (other.m_value), m_loc (other.m_loc) {}
+    m_value (other.m_value), m_loc (other.m_loc),
+    m_char_literal_p (other.m_char_literal_p) {}
 
   /* Implicit conversions to tree.  */
   operator tree () const { return m_value; }
@@ -91,9 +96,12 @@ public:
     set_location (make_location (m_loc, start, finish));
   }
 
+  bool is_char_literal_p () const { return m_char_literal_p; }
+
  private:
   tree m_value;
   location_t m_loc;
+  bool m_char_literal_p;
 };
 
 inline bool
diff --git gcc/cp/parser.c gcc/cp/parser.c
index ca9f8b9..405ba14 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -4756,6 +4756,7 @@ cp_parser_primary_expression (cp_parser *parser,
 			      cp_id_kind *idk)
 {
   cp_token *token = NULL;
+  bool char_literal_p = false;
 
   /* Assume the primary expression is not an id-expression.  */
   *idk = CP_ID_KIND_NONE;
@@ -4777,6 +4778,8 @@ cp_parser_primary_expression (cp_parser *parser,
     case CPP_CHAR32:
     case CPP_WCHAR:
     case CPP_UTF8CHAR:
+      char_literal_p = true;
+      /* FALLTHRU */
     case CPP_NUMBER:
     case CPP_PREPARSED_EXPR:
       if (TREE_CODE (token->u.value) == USERDEF_LITERAL)
@@ -4832,7 +4835,7 @@ cp_parser_primary_expression (cp_parser *parser,
 	  if (!cast_p)
 	    cp_parser_non_integral_constant_expression (parser, NIC_FLOAT);
 	}
-      return cp_expr (token->u.value, token->location);
+      return cp_expr (token->u.value, token->location, char_literal_p);
 
     case CPP_CHAR_USERDEF:
     case CPP_CHAR16_USERDEF:
@@ -8924,6 +8927,31 @@ cp_parser_binary_expression (cp_parser* parser, bool cast_p,
 	warn_logical_not_parentheses (current.loc, current.tree_type,
 				      current.lhs, maybe_constant_value (rhs));
 
+      /* Warn for ptr == '\0', it's likely that it should've been ptr[0].  */
+      if (current.tree_type == EQ_EXPR || current.tree_type == NE_EXPR)
+	{
+	  if (TREE_TYPE (current.lhs.get_value ())
+	      && POINTER_TYPE_P (TREE_TYPE (current.lhs.get_value ()))
+	      && integer_zerop (rhs.get_value ())
+	      && !TREE_OVERFLOW (rhs.get_value ())
+	      && rhs.is_char_literal_p ()
+	      && warning_at (current.loc, OPT_Wpointer_compare,
+			     "comparison between pointer and zero character "
+			     "literal"))
+	    inform (current.lhs.get_location (),
+		    "did you mean to dereference the pointer?");
+	  else if (TREE_TYPE (rhs.get_value ())
+		   && POINTER_TYPE_P (TREE_TYPE (rhs.get_value ()))
+		   && integer_zerop (current.lhs.get_value ())
+		   && !TREE_OVERFLOW (current.lhs.get_value ())
+		   && current.lhs.is_char_literal_p ()
+		   && warning_at (current.loc, OPT_Wpointer_compare,
+				  "comparison between pointer and zero "
+				  "character literal"))
+	    inform (rhs.get_location (),
+		    "did you mean to dereference the pointer?");
+
+	}
       overload = NULL;
 
       location_t combined_loc = make_location (current.loc,
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index b2eaea7..734dcbc 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -287,7 +287,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wpacked  -Wpacked-bitfield-compat  -Wpadded @gol
 -Wparentheses -Wno-pedantic-ms-format @gol
 -Wplacement-new -Wplacement-new=@var{n} @gol
--Wpointer-arith  -Wno-pointer-to-int-cast @gol
+-Wpointer-arith  -Wpointer-compare  -Wno-pointer-to-int-cast @gol
 -Wno-pragmas -Wredundant-decls  -Wno-return-local-addr @gol
 -Wreturn-type  -Wsequence-point  -Wshadow  -Wno-shadow-ivar @gol
 -Wshift-overflow -Wshift-overflow=@var{n} @gol
@@ -5136,6 +5136,20 @@ convenience in calculations with @code{void *} pointers and pointers
 to functions.  In C++, warn also when an arithmetic operation involves
 @code{NULL}.  This warning is also enabled by @option{-Wpedantic}.
 
+@item -Wpointer-compare
+@opindex Wpointer-compare
+@opindex Wno-pointer-compare
+Warn if a pointer is compared with a zero character constant.  This usually
+means that the pointer was meant to be dereferenced.  For example:
+
+@smallexample
+const char *p = foo ();
+if (p == '\0')
+  return 42;
+@end smallexample
+
+This warning is enabled by default.
+
 @item -Wtype-limits
 @opindex Wtype-limits
 @opindex Wno-type-limits
diff --git gcc/testsuite/c-c++-common/Wpointer-compare-1.c gcc/testsuite/c-c++-common/Wpointer-compare-1.c
index e69de29..80da85a 100644
--- gcc/testsuite/c-c++-common/Wpointer-compare-1.c
+++ gcc/testsuite/c-c++-common/Wpointer-compare-1.c
@@ -0,0 +1,81 @@
+/* PR c++/64767 */
+/* { dg-do compile } */
+/* { dg-options "-Wpointer-compare" } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+
+int
+f1 (int *p, int **q)
+{
+  int r = 0;
+
+  r += p == '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p == L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p == u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p == U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  r += '\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += '\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  r += q == '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q == L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q == u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q == U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  r += '\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += '\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  return r;
+}
+
+int
+f2 (int *p)
+{
+  int r = 0;
+
+  /* Keep quiet.  */
+  r += p == (void *) 0;
+  r += p != (void *) 0;
+  r += (void *) 0 == p;
+  r += (void *) 0 != p;
+
+  r += p == 0;
+  r += p != 0;
+  r += 0 == p;
+  r += 0 != p;
+
+  return r;
+}
+
+int
+f3 (int *p)
+{
+  int r = 0;
+
+  r += p == (char) 0;
+  r += p != (char) 0;
+
+  r += (char) 0 == p;
+  r += (char) 0 != p;
+
+  return r;
+}

	Marek

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Jason Merrill]

On Sat, Sep 10, 2016 at 10:58 AM, Marek Polacek <polacek@redhat.com> wrote:
> Spurred by the recent <http://www.viva64.com/en/b/0425/> findings, I decided to
> implement a warning that warns when a pointer is compared with a zero character
> literal (constant), because this isn't likely to be intended.  So e.g.
>
>   ptr == L'\0'
>
> is probably wrong and should've been written as
>
>   ptr[0] == L'\0'
>
> Jonathan pointed out that this would actually be invalid C++11 since pointer
> conversions are only allowed for integer literals, not char literals.

Ah, indeed.  And if we fix that, we get an error rather than a
warning.  Maybe let's handle this by wrapping character literals in a
redundant NOP_EXPR?

Jason

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Marek Polacek]

On Wed, Sep 14, 2016 at 12:07:49AM -0400, Jason Merrill wrote:
> On Sat, Sep 10, 2016 at 10:58 AM, Marek Polacek <polacek@redhat.com> wrote:
> > Spurred by the recent <http://www.viva64.com/en/b/0425/> findings, I decided to
> > implement a warning that warns when a pointer is compared with a zero character
> > literal (constant), because this isn't likely to be intended.  So e.g.
> >
> >   ptr == L'\0'
> >
> > is probably wrong and should've been written as
> >
> >   ptr[0] == L'\0'
> >
> > Jonathan pointed out that this would actually be invalid C++11 since pointer
> > conversions are only allowed for integer literals, not char literals.
> 
> Ah, indeed.  And if we fix that, we get an error rather than a
> warning.  Maybe let's handle this by wrapping character literals in a
> redundant NOP_EXPR?

So I've tried.  Wrapping character literals in a NOP_EXPR would make us
error on that comparison (good), but it breaks -Wchar-subscripts, which
could be solved by adding STRIP_NOPS, but unfortunately it breaks even
-Wmemset-transposed-args: '\0' would become (char) '\0', which is not a
literal zero anymore.  And if I do sth like
+       if (TREE_CODE (arg2) == NOP_EXPR
+           && TREE_TYPE (arg2) == TREE_TYPE (TREE_OPERAND (arg2, 0)))
+         arg2 = TREE_OPERAND (arg2, 0);
then (int) 0 would became a literal zero and we'd warn when not appropriate.

We should also error for e.g. void *p = '\0'; but the problem with the
NOP_EXPR cast is that convert_for_init strips nops, so we wouldn't reject
this code.

So it seems we may need some CHARACTER_CST or somesuch?

Note that we should also take boolean-literals into account.

	Marek

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Jason Merrill]

On Thu, Sep 15, 2016 at 8:16 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Wed, Sep 14, 2016 at 12:07:49AM -0400, Jason Merrill wrote:
>> On Sat, Sep 10, 2016 at 10:58 AM, Marek Polacek <polacek@redhat.com> wrote:
>> > Spurred by the recent <http://www.viva64.com/en/b/0425/> findings, I decided to
>> > implement a warning that warns when a pointer is compared with a zero character
>> > literal (constant), because this isn't likely to be intended.  So e.g.
>> >
>> >   ptr == L'\0'
>> >
>> > is probably wrong and should've been written as
>> >
>> >   ptr[0] == L'\0'
>> >
>> > Jonathan pointed out that this would actually be invalid C++11 since pointer
>> > conversions are only allowed for integer literals, not char literals.
>>
>> Ah, indeed.  And if we fix that, we get an error rather than a
>> warning.  Maybe let's handle this by wrapping character literals in a
>> redundant NOP_EXPR?
>
> So I've tried.  Wrapping character literals in a NOP_EXPR would make us
> error on that comparison (good), but it breaks -Wchar-subscripts, which
> could be solved by adding STRIP_NOPS, but unfortunately it breaks even
> -Wmemset-transposed-args: '\0' would become (char) '\0', which is not a
> literal zero anymore.  And if I do sth like
> +       if (TREE_CODE (arg2) == NOP_EXPR
> +           && TREE_TYPE (arg2) == TREE_TYPE (TREE_OPERAND (arg2, 0)))
> +         arg2 = TREE_OPERAND (arg2, 0);
> then (int) 0 would became a literal zero and we'd warn when not appropriate.
>
> We should also error for e.g. void *p = '\0'; but the problem with the
> NOP_EXPR cast is that convert_for_init strips nops, so we wouldn't reject
> this code.

Good point.

> So it seems we may need some CHARACTER_CST or somesuch?

I suppose that an INTEGER_CST of character type is necessarily a
character constant, so adding a check for !char_type_p ought to do the
trick.

> Note that we should also take boolean-literals into account.

We already check for INTEGER_TYPE, so boolean literals should already
be excluded.

Jason

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 262 bytes --]

On Mon, Sep 19, 2016 at 2:49 PM, Jason Merrill <jason@redhat.com> wrote:
> I suppose that an INTEGER_CST of character type is necessarily a
> character constant, so adding a check for !char_type_p ought to do the
> trick.

Indeed it does.  I'm checking this in:

[-- Attachment #2: char903.diff --]
[-- Type: text/x-patch, Size: 918 bytes --]

commit d2f237ef0f63b3ee3da79bcbfad08fedb325d554
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Sep 21 10:58:39 2016 -0400

            Core 903
            * call.c (null_ptr_cst_p): Check char_type_p.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 393aab9..2804bd8 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -527,6 +527,7 @@ null_ptr_cst_p (tree t)
     {
       /* Core issue 903 says only literal 0 is a null pointer constant.  */
       if (TREE_CODE (type) == INTEGER_TYPE
+	  && !char_type_p (type)
 	  && TREE_CODE (t) == INTEGER_CST
 	  && integer_zerop (t)
 	  && !TREE_OVERFLOW (t))
diff --git a/gcc/testsuite/g++.dg/cpp0x/nullptr36.C b/gcc/testsuite/g++.dg/cpp0x/nullptr36.C
new file mode 100644
index 0000000..5f43881
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/nullptr36.C
@@ -0,0 +1,3 @@
+// { dg-do compile { target c++11 } }
+
+void *p = '\0';			// { dg-error "invalid conversion" }

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Marek Polacek]

On Wed, Sep 21, 2016 at 03:52:09PM -0400, Jason Merrill wrote:
> On Mon, Sep 19, 2016 at 2:49 PM, Jason Merrill <jason@redhat.com> wrote:
> > I suppose that an INTEGER_CST of character type is necessarily a
> > character constant, so adding a check for !char_type_p ought to do the
> > trick.
> 
> Indeed it does.  I'm checking this in:

Nice, thanks.  What about the original patch?  We still need to warn
(or error for C++11) for pointer comparisons.

	Marek

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Jason Merrill]

On Fri, Sep 23, 2016 at 9:15 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Wed, Sep 21, 2016 at 03:52:09PM -0400, Jason Merrill wrote:
>> On Mon, Sep 19, 2016 at 2:49 PM, Jason Merrill <jason@redhat.com> wrote:
>> > I suppose that an INTEGER_CST of character type is necessarily a
>> > character constant, so adding a check for !char_type_p ought to do the
>> > trick.
>>
>> Indeed it does.  I'm checking this in:
>
> Nice, thanks.  What about the original patch?  We still need to warn
> (or error for C++11) for pointer comparisons.

If we still accept pointer comparisons in C++, that's another bug with
treating \0 as a null pointer constant.  This seems to be because
ocp_convert of \0 to int produces an INTEGER_CST indistinguishable
from literal 0.

Jason

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Marek Polacek]

On Fri, Sep 23, 2016 at 10:31:33AM -0400, Jason Merrill wrote:
> On Fri, Sep 23, 2016 at 9:15 AM, Marek Polacek <polacek@redhat.com> wrote:
> > On Wed, Sep 21, 2016 at 03:52:09PM -0400, Jason Merrill wrote:
> >> On Mon, Sep 19, 2016 at 2:49 PM, Jason Merrill <jason@redhat.com> wrote:
> >> > I suppose that an INTEGER_CST of character type is necessarily a
> >> > character constant, so adding a check for !char_type_p ought to do the
> >> > trick.
> >>
> >> Indeed it does.  I'm checking this in:
> >
> > Nice, thanks.  What about the original patch?  We still need to warn
> > (or error for C++11) for pointer comparisons.
> 
> If we still accept pointer comparisons in C++, that's another bug with
> treating \0 as a null pointer constant.  This seems to be because
> ocp_convert of \0 to int produces an INTEGER_CST indistinguishable
> from literal 0.

I was trying to fix this in ocp_convert, by using NOP_EXPRs, but that wasn't
successful.  But since we're interested in ==/!=, I think this can be fixed
easily in cp_build_binary_op.  Actually, all that seems to be needed is using
orig_op as the argument to null_ptr_cst_p, but that wouldn't give the correct
diagnostics, so I did this.  By checking orig_op we can see if the operands are
character literals or not, because orig_op is an operand before the default
conversions.

Curiously, nothing in the testsuite broke.

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

2016-09-30  Marek Polacek  <polacek@redhat.com>

	Core 903
	* typeck.c (cp_build_binary_op) [EQ_EXPR]: Diagnose invalid pointer
	conversions.

	* g++.dg/cpp0x/nullptr37.C: New test.

diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 617ca55..2e6f44e 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -4584,6 +4584,14 @@ cp_build_binary_op (location_t location,
 	  else
 	    result_type = type0;
 
+	  if (null_ptr_cst_p (op1) && !null_ptr_cst_p (orig_op1))
+	    {
+	      if (complain & tf_error)
+		permerror (input_location, "ISO C++11 only allows pointer "
+			   "conversions for integer literals");
+	      else
+		return error_mark_node;
+	    }
 	  warn_for_null_address (location, op0, complain);
 	}
       else if (((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1))
@@ -4598,6 +4606,14 @@ cp_build_binary_op (location_t location,
 	  else
 	    result_type = type1;
 
+	  if (null_ptr_cst_p (op0) && !null_ptr_cst_p (orig_op0))
+	    {
+	      if (complain & tf_error)
+		permerror (input_location, "ISO C++11 only allows pointer "
+			   "conversions for integer literals");
+	      else
+		return error_mark_node;
+	    }
 	  warn_for_null_address (location, op1, complain);
 	}
       else if ((code0 == POINTER_TYPE && code1 == POINTER_TYPE)
diff --git gcc/testsuite/g++.dg/cpp0x/nullptr37.C gcc/testsuite/g++.dg/cpp0x/nullptr37.C
index e69de29..17c33d1 100644
--- gcc/testsuite/g++.dg/cpp0x/nullptr37.C
+++ gcc/testsuite/g++.dg/cpp0x/nullptr37.C
@@ -0,0 +1,78 @@
+/* PR c++/64767 */
+// { dg-do compile { target c++11 } }
+
+int
+f1 (int *p, int **q)
+{
+  int r = 0;
+
+  r += p == '\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += p == L'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += p == u'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += p == U'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += p != '\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += p != L'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += p != u'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += p != U'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+
+  r += '\0' == p; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += L'\0' == p; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += u'\0' == p; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += U'\0' == p; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += '\0' != p; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += L'\0' != p; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += u'\0' != p; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += U'\0' != p; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+
+  r += q == '\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += q == L'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += q == u'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += q == U'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += q != '\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += q != L'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += q != u'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += q != U'\0'; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+
+  r += '\0' == q; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += L'\0' == q; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += u'\0' == q; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += U'\0' == q; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += '\0' != q; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += L'\0' != q; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += u'\0' != q; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += U'\0' != q; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+
+  return r;
+}
+
+int
+f2 (int *p)
+{
+  int r = 0;
+
+  r += p == (void *) 0;
+  r += p != (void *) 0;
+  r += (void *) 0 == p;
+  r += (void *) 0 != p;
+
+  r += p == 0;
+  r += p != 0;
+  r += 0 == p;
+  r += 0 != p;
+
+  return r;
+}
+
+int
+f3 (int *p)
+{
+  int r = 0;
+
+  r += p == (char) 0; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += p != (char) 0; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+
+  r += (char) 0 == p; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+  r += (char) 0 != p; /* { dg-error "ISO C\\+\\+11 only allows pointer conversions for integer literals" } */
+
+  return r;
+}


	Marek

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Martin Sebor]

> +		permerror (input_location, "ISO C++11 only allows pointer "
> +			   "conversions for integer literals");

FWIW, I think it would be clearer to either mention the currently
selected language version or leave it out completely rather than
hardcoding C++11.  When the user specifies -std=c++14 (or later),
or when that's the current version used by the compiler, a warning
that tells them what C++ 11 allows isn't really relevant (or
meaningful), and becomes less so as time goes on.

Martin

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Martin Sebor]

On 09/30/2016 11:05 AM, Martin Sebor wrote:
>> +        permerror (input_location, "ISO C++11 only allows pointer "
>> +               "conversions for integer literals");
>
> FWIW, I think it would be clearer to either mention the currently
> selected language version or leave it out completely rather than
> hardcoding C++11.  When the user specifies -std=c++14 (or later),
> or when that's the current version used by the compiler, a warning
> that tells them what C++ 11 allows isn't really relevant (or
> meaningful), and becomes less so as time goes on.

Btw., I realize there are other instances where the C++ version
has been hardcoded and so I should have made it clear that I meant
my comment as a general suggestion, not as an objection to the patch.
Sorry if that wasn't apparent.

I've quickly surveyed some of the existing messages that mention
a C++ version and found instances where it applies only to the
referenced version of the standard and prior but not later versions. 
For example:

   msgid "in C++98 %q+D may not be static because it is a member of a union"
   msgid "in C++03 a class-key must be used when declaring a friend"

But I also came across another form that may resolve the concern
I raised: mentioning the version along with whether it's the first
or last that supports (or doesn't support) the construct, such as
"C++11 and above."  Here's one example of it:

   msgid "in C++11 and above a default constructor can be explicit"

It might be worth to come up with a convention to adopt in new
messages and (gradually) change what's already there to use it.

Martin

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Marek Polacek]

On Fri, Sep 30, 2016 at 01:08:19PM -0600, Martin Sebor wrote:
> On 09/30/2016 11:05 AM, Martin Sebor wrote:
> > > +        permerror (input_location, "ISO C++11 only allows pointer "
> > > +               "conversions for integer literals");
> > 
> > FWIW, I think it would be clearer to either mention the currently
> > selected language version or leave it out completely rather than
> > hardcoding C++11.  When the user specifies -std=c++14 (or later),
> > or when that's the current version used by the compiler, a warning
> > that tells them what C++ 11 allows isn't really relevant (or
> > meaningful), and becomes less so as time goes on.
> 
> Btw., I realize there are other instances where the C++ version
> has been hardcoded and so I should have made it clear that I meant
> my comment as a general suggestion, not as an objection to the patch.
> Sorry if that wasn't apparent.
> 
> I've quickly surveyed some of the existing messages that mention
> a C++ version and found instances where it applies only to the
> referenced version of the standard and prior but not later versions. For
> example:
> 
>   msgid "in C++98 %q+D may not be static because it is a member of a union"
>   msgid "in C++03 a class-key must be used when declaring a friend"
> 
> But I also came across another form that may resolve the concern
> I raised: mentioning the version along with whether it's the first
> or last that supports (or doesn't support) the construct, such as
> "C++11 and above."  Here's one example of it:
> 
>   msgid "in C++11 and above a default constructor can be explicit"
> 
> It might be worth to come up with a convention to adopt in new
> messages and (gradually) change what's already there to use it.

Maybe, no strong opinions, I'll just go with whatever people prefer here.
Though I guess I like those explicit versions because you know when a
particular thing changed.  And when you're using -std=c++14 and the
compiler says C++11, I think that's ok, because C++14 implies C++11.

	Marek

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Martin Sebor]

On 09/30/2016 01:40 PM, Marek Polacek wrote:
> On Fri, Sep 30, 2016 at 01:08:19PM -0600, Martin Sebor wrote:
>> On 09/30/2016 11:05 AM, Martin Sebor wrote:
>>>> +        permerror (input_location, "ISO C++11 only allows pointer "
>>>> +               "conversions for integer literals");
>>>
>>> FWIW, I think it would be clearer to either mention the currently
>>> selected language version or leave it out completely rather than
>>> hardcoding C++11.  When the user specifies -std=c++14 (or later),
>>> or when that's the current version used by the compiler, a warning
>>> that tells them what C++ 11 allows isn't really relevant (or
>>> meaningful), and becomes less so as time goes on.
>>
>> Btw., I realize there are other instances where the C++ version
>> has been hardcoded and so I should have made it clear that I meant
>> my comment as a general suggestion, not as an objection to the patch.
>> Sorry if that wasn't apparent.
>>
>> I've quickly surveyed some of the existing messages that mention
>> a C++ version and found instances where it applies only to the
>> referenced version of the standard and prior but not later versions. For
>> example:
>>
>>   msgid "in C++98 %q+D may not be static because it is a member of a union"
>>   msgid "in C++03 a class-key must be used when declaring a friend"
>>
>> But I also came across another form that may resolve the concern
>> I raised: mentioning the version along with whether it's the first
>> or last that supports (or doesn't support) the construct, such as
>> "C++11 and above."  Here's one example of it:
>>
>>   msgid "in C++11 and above a default constructor can be explicit"
>>
>> It might be worth to come up with a convention to adopt in new
>> messages and (gradually) change what's already there to use it.
>
> Maybe, no strong opinions, I'll just go with whatever people prefer here.
> Though I guess I like those explicit versions because you know when a
> particular thing changed.  And when you're using -std=c++14 and the
> compiler says C++11, I think that's ok, because C++14 implies C++11.

I agree that mentioning the (first or last) version can be helpful.
Does adopting the "C++11 and later" and "C++11 and earlier" phrasing
sound good then?  If no one objects I'll add it to the Guidelines for
C/C++ FE diagnostics on the Wiki.

(I changed "above" to "later" because it pairs better with "earlier"
than "below" would , and because it seems to be more natural when
referring to the year the version a standard was ratified.)

Martin

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Jason Merrill]

On Fri, Sep 30, 2016 at 12:43 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Fri, Sep 23, 2016 at 10:31:33AM -0400, Jason Merrill wrote:
>> On Fri, Sep 23, 2016 at 9:15 AM, Marek Polacek <polacek@redhat.com> wrote:
>> > On Wed, Sep 21, 2016 at 03:52:09PM -0400, Jason Merrill wrote:
>> >> On Mon, Sep 19, 2016 at 2:49 PM, Jason Merrill <jason@redhat.com> wrote:
>> >> > I suppose that an INTEGER_CST of character type is necessarily a
>> >> > character constant, so adding a check for !char_type_p ought to do the
>> >> > trick.
>> >>
>> >> Indeed it does.  I'm checking this in:
>> >
>> > Nice, thanks.  What about the original patch?  We still need to warn
>> > (or error for C++11) for pointer comparisons.
>>
>> If we still accept pointer comparisons in C++, that's another bug with
>> treating \0 as a null pointer constant.  This seems to be because
>> ocp_convert of \0 to int produces an INTEGER_CST indistinguishable
>> from literal 0.
>
> I was trying to fix this in ocp_convert, by using NOP_EXPRs, but that wasn't
> successful.  But since we're interested in ==/!=, I think this can be fixed
> easily in cp_build_binary_op.  Actually, all that seems to be needed is using
> orig_op as the argument to null_ptr_cst_p, but that wouldn't give the correct
> diagnostics, so I did this.  By checking orig_op we can see if the operands are
> character literals or not, because orig_op is an operand before the default
> conversions.

What is wrong about the diagnostic from just using orig_op?  "ISO C++
forbids comparison between pointer and integer" seems fine to me, and
will help the user to realize that they need to index off the pointer.

I see that some of the calls to null_ptr_cst_p in cp_build_binary_op
have already been changed to check orig_op*, but not all.  Let's
update the remaining calls, that should do the trick without adding a
new error.

Jason

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Marek Polacek]

On Fri, Sep 30, 2016 at 05:48:03PM -0400, Jason Merrill wrote:
> On Fri, Sep 30, 2016 at 12:43 PM, Marek Polacek <polacek@redhat.com> wrote:
> > On Fri, Sep 23, 2016 at 10:31:33AM -0400, Jason Merrill wrote:
> >> On Fri, Sep 23, 2016 at 9:15 AM, Marek Polacek <polacek@redhat.com> wrote:
> >> > On Wed, Sep 21, 2016 at 03:52:09PM -0400, Jason Merrill wrote:
> >> >> On Mon, Sep 19, 2016 at 2:49 PM, Jason Merrill <jason@redhat.com> wrote:
> >> >> > I suppose that an INTEGER_CST of character type is necessarily a
> >> >> > character constant, so adding a check for !char_type_p ought to do the
> >> >> > trick.
> >> >>
> >> >> Indeed it does.  I'm checking this in:
> >> >
> >> > Nice, thanks.  What about the original patch?  We still need to warn
> >> > (or error for C++11) for pointer comparisons.
> >>
> >> If we still accept pointer comparisons in C++, that's another bug with
> >> treating \0 as a null pointer constant.  This seems to be because
> >> ocp_convert of \0 to int produces an INTEGER_CST indistinguishable
> >> from literal 0.
> >
> > I was trying to fix this in ocp_convert, by using NOP_EXPRs, but that wasn't
> > successful.  But since we're interested in ==/!=, I think this can be fixed
> > easily in cp_build_binary_op.  Actually, all that seems to be needed is using
> > orig_op as the argument to null_ptr_cst_p, but that wouldn't give the correct
> > diagnostics, so I did this.  By checking orig_op we can see if the operands are
> > character literals or not, because orig_op is an operand before the default
> > conversions.
> 
> What is wrong about the diagnostic from just using orig_op?  "ISO C++
> forbids comparison between pointer and integer" seems fine to me, and
> will help the user to realize that they need to index off the pointer.
> 
> I see that some of the calls to null_ptr_cst_p in cp_build_binary_op
> have already been changed to check orig_op*, but not all.  Let's
> update the remaining calls, that should do the trick without adding a
> new error.

Here you go:

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

2016-10-01  Marek Polacek  <polacek@redhat.com>

	Core 903
	* typeck.c (cp_build_binary_op): Pass original operands to
	null_ptr_cst_p, not those after the default conversions.

	* g++.dg/cpp0x/nullptr37.C: New test.

diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 617ca55..8b780be 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -4573,7 +4573,7 @@ cp_build_binary_op (location_t location,
 	      || code1 == COMPLEX_TYPE || code1 == ENUMERAL_TYPE))
 	short_compare = 1;
       else if (((code0 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type0))
-		&& null_ptr_cst_p (op1))
+		&& null_ptr_cst_p (orig_op1))
 	       /* Handle, eg, (void*)0 (c++/43906), and more.  */
 	       || (code0 == POINTER_TYPE
 		   && TYPE_PTR_P (type1) && integer_zerop (op1)))
@@ -4587,7 +4587,7 @@ cp_build_binary_op (location_t location,
 	  warn_for_null_address (location, op0, complain);
 	}
       else if (((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1))
-		&& null_ptr_cst_p (op0))
+		&& null_ptr_cst_p (orig_op0))
 	       /* Handle, eg, (void*)0 (c++/43906), and more.  */
 	       || (code1 == POINTER_TYPE
 		   && TYPE_PTR_P (type0) && integer_zerop (op0)))
@@ -4604,7 +4604,7 @@ cp_build_binary_op (location_t location,
 	       || (TYPE_PTRDATAMEM_P (type0) && TYPE_PTRDATAMEM_P (type1)))
 	result_type = composite_pointer_type (type0, type1, op0, op1,
 					      CPO_COMPARISON, complain);
-      else if (null_ptr_cst_p (op0) && null_ptr_cst_p (op1))
+      else if (null_ptr_cst_p (orig_op0) && null_ptr_cst_p (orig_op1))
 	/* One of the operands must be of nullptr_t type.  */
         result_type = TREE_TYPE (nullptr_node);
       else if (code0 == POINTER_TYPE && code1 == INTEGER_TYPE)
@@ -4623,7 +4623,7 @@ cp_build_binary_op (location_t location,
           else
             return error_mark_node;
 	}
-      else if (TYPE_PTRMEMFUNC_P (type0) && null_ptr_cst_p (op1))
+      else if (TYPE_PTRMEMFUNC_P (type0) && null_ptr_cst_p (orig_op1))
 	{
 	  if (TARGET_PTRMEMFUNC_VBIT_LOCATION
 	      == ptrmemfunc_vbit_in_delta)
@@ -4664,7 +4664,7 @@ cp_build_binary_op (location_t location,
 	    }
 	  result_type = TREE_TYPE (op0);
 	}
-      else if (TYPE_PTRMEMFUNC_P (type1) && null_ptr_cst_p (op0))
+      else if (TYPE_PTRMEMFUNC_P (type1) && null_ptr_cst_p (orig_op0))
 	return cp_build_binary_op (location, code, op1, op0, complain);
       else if (TYPE_PTRMEMFUNC_P (type0) && TYPE_PTRMEMFUNC_P (type1))
 	{
@@ -4877,21 +4877,21 @@ cp_build_binary_op (location_t location,
       else if (code0 == POINTER_TYPE && code1 == POINTER_TYPE)
 	result_type = composite_pointer_type (type0, type1, op0, op1,
 					      CPO_COMPARISON, complain);
-      else if (code0 == POINTER_TYPE && null_ptr_cst_p (op1))
+      else if (code0 == POINTER_TYPE && null_ptr_cst_p (orig_op1))
 	{
 	  result_type = type0;
 	  if (extra_warnings && (complain & tf_warning))
 	    warning (OPT_Wextra,
 		     "ordered comparison of pointer with integer zero");
 	}
-      else if (code1 == POINTER_TYPE && null_ptr_cst_p (op0))
+      else if (code1 == POINTER_TYPE && null_ptr_cst_p (orig_op0))
 	{
 	  result_type = type1;
 	  if (extra_warnings && (complain & tf_warning))
 	    warning (OPT_Wextra,
 		     "ordered comparison of pointer with integer zero");
 	}
-      else if (null_ptr_cst_p (op0) && null_ptr_cst_p (op1))
+      else if (null_ptr_cst_p (orig_op0) && null_ptr_cst_p (orig_op1))
 	/* One of the operands must be of nullptr_t type.  */
         result_type = TREE_TYPE (nullptr_node);
       else if (code0 == POINTER_TYPE && code1 == INTEGER_TYPE)
diff --git gcc/testsuite/g++.dg/cpp0x/nullptr37.C gcc/testsuite/g++.dg/cpp0x/nullptr37.C
index e69de29..e746a28 100644
--- gcc/testsuite/g++.dg/cpp0x/nullptr37.C
+++ gcc/testsuite/g++.dg/cpp0x/nullptr37.C
@@ -0,0 +1,78 @@
+/* PR c++/64767 */
+// { dg-do compile { target c++11 } }
+
+int
+f1 (int *p, int **q)
+{
+  int r = 0;
+
+  r += p == '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += p == L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += p == u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += p == U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += p != '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += p != L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += p != u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += p != U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+
+  r += '\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += L'\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += u'\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += U'\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += '\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += L'\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += u'\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += U'\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+
+  r += q == '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += q == L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += q == u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += q == U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += q != '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += q != L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += q != u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += q != U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+
+  r += '\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += L'\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += u'\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += U'\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += '\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += L'\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += u'\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += U'\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+
+  return r;
+}
+
+int
+f2 (int *p)
+{
+  int r = 0;
+
+  r += p == (void *) 0;
+  r += p != (void *) 0;
+  r += (void *) 0 == p;
+  r += (void *) 0 != p;
+
+  r += p == 0;
+  r += p != 0;
+  r += 0 == p;
+  r += 0 != p;
+
+  return r;
+}
+
+int
+f3 (int *p)
+{
+  int r = 0;
+
+  r += p == (char) 0; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += p != (char) 0; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+
+  r += (char) 0 == p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+  r += (char) 0 != p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
+
+  return r;
+}

	Marek

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Jason Merrill]

OK, thanks.

On Sat, Oct 1, 2016 at 10:16 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Fri, Sep 30, 2016 at 05:48:03PM -0400, Jason Merrill wrote:
>> On Fri, Sep 30, 2016 at 12:43 PM, Marek Polacek <polacek@redhat.com> wrote:
>> > On Fri, Sep 23, 2016 at 10:31:33AM -0400, Jason Merrill wrote:
>> >> On Fri, Sep 23, 2016 at 9:15 AM, Marek Polacek <polacek@redhat.com> wrote:
>> >> > On Wed, Sep 21, 2016 at 03:52:09PM -0400, Jason Merrill wrote:
>> >> >> On Mon, Sep 19, 2016 at 2:49 PM, Jason Merrill <jason@redhat.com> wrote:
>> >> >> > I suppose that an INTEGER_CST of character type is necessarily a
>> >> >> > character constant, so adding a check for !char_type_p ought to do the
>> >> >> > trick.
>> >> >>
>> >> >> Indeed it does.  I'm checking this in:
>> >> >
>> >> > Nice, thanks.  What about the original patch?  We still need to warn
>> >> > (or error for C++11) for pointer comparisons.
>> >>
>> >> If we still accept pointer comparisons in C++, that's another bug with
>> >> treating \0 as a null pointer constant.  This seems to be because
>> >> ocp_convert of \0 to int produces an INTEGER_CST indistinguishable
>> >> from literal 0.
>> >
>> > I was trying to fix this in ocp_convert, by using NOP_EXPRs, but that wasn't
>> > successful.  But since we're interested in ==/!=, I think this can be fixed
>> > easily in cp_build_binary_op.  Actually, all that seems to be needed is using
>> > orig_op as the argument to null_ptr_cst_p, but that wouldn't give the correct
>> > diagnostics, so I did this.  By checking orig_op we can see if the operands are
>> > character literals or not, because orig_op is an operand before the default
>> > conversions.
>>
>> What is wrong about the diagnostic from just using orig_op?  "ISO C++
>> forbids comparison between pointer and integer" seems fine to me, and
>> will help the user to realize that they need to index off the pointer.
>>
>> I see that some of the calls to null_ptr_cst_p in cp_build_binary_op
>> have already been changed to check orig_op*, but not all.  Let's
>> update the remaining calls, that should do the trick without adding a
>> new error.
>
> Here you go:
>
> Bootstrapped/regtested on x86_64-linux and ppc64-linux, ok for trunk?
>
> 2016-10-01  Marek Polacek  <polacek@redhat.com>
>
>         Core 903
>         * typeck.c (cp_build_binary_op): Pass original operands to
>         null_ptr_cst_p, not those after the default conversions.
>
>         * g++.dg/cpp0x/nullptr37.C: New test.
>
> diff --git gcc/cp/typeck.c gcc/cp/typeck.c
> index 617ca55..8b780be 100644
> --- gcc/cp/typeck.c
> +++ gcc/cp/typeck.c
> @@ -4573,7 +4573,7 @@ cp_build_binary_op (location_t location,
>               || code1 == COMPLEX_TYPE || code1 == ENUMERAL_TYPE))
>         short_compare = 1;
>        else if (((code0 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type0))
> -               && null_ptr_cst_p (op1))
> +               && null_ptr_cst_p (orig_op1))
>                /* Handle, eg, (void*)0 (c++/43906), and more.  */
>                || (code0 == POINTER_TYPE
>                    && TYPE_PTR_P (type1) && integer_zerop (op1)))
> @@ -4587,7 +4587,7 @@ cp_build_binary_op (location_t location,
>           warn_for_null_address (location, op0, complain);
>         }
>        else if (((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1))
> -               && null_ptr_cst_p (op0))
> +               && null_ptr_cst_p (orig_op0))
>                /* Handle, eg, (void*)0 (c++/43906), and more.  */
>                || (code1 == POINTER_TYPE
>                    && TYPE_PTR_P (type0) && integer_zerop (op0)))
> @@ -4604,7 +4604,7 @@ cp_build_binary_op (location_t location,
>                || (TYPE_PTRDATAMEM_P (type0) && TYPE_PTRDATAMEM_P (type1)))
>         result_type = composite_pointer_type (type0, type1, op0, op1,
>                                               CPO_COMPARISON, complain);
> -      else if (null_ptr_cst_p (op0) && null_ptr_cst_p (op1))
> +      else if (null_ptr_cst_p (orig_op0) && null_ptr_cst_p (orig_op1))
>         /* One of the operands must be of nullptr_t type.  */
>          result_type = TREE_TYPE (nullptr_node);
>        else if (code0 == POINTER_TYPE && code1 == INTEGER_TYPE)
> @@ -4623,7 +4623,7 @@ cp_build_binary_op (location_t location,
>            else
>              return error_mark_node;
>         }
> -      else if (TYPE_PTRMEMFUNC_P (type0) && null_ptr_cst_p (op1))
> +      else if (TYPE_PTRMEMFUNC_P (type0) && null_ptr_cst_p (orig_op1))
>         {
>           if (TARGET_PTRMEMFUNC_VBIT_LOCATION
>               == ptrmemfunc_vbit_in_delta)
> @@ -4664,7 +4664,7 @@ cp_build_binary_op (location_t location,
>             }
>           result_type = TREE_TYPE (op0);
>         }
> -      else if (TYPE_PTRMEMFUNC_P (type1) && null_ptr_cst_p (op0))
> +      else if (TYPE_PTRMEMFUNC_P (type1) && null_ptr_cst_p (orig_op0))
>         return cp_build_binary_op (location, code, op1, op0, complain);
>        else if (TYPE_PTRMEMFUNC_P (type0) && TYPE_PTRMEMFUNC_P (type1))
>         {
> @@ -4877,21 +4877,21 @@ cp_build_binary_op (location_t location,
>        else if (code0 == POINTER_TYPE && code1 == POINTER_TYPE)
>         result_type = composite_pointer_type (type0, type1, op0, op1,
>                                               CPO_COMPARISON, complain);
> -      else if (code0 == POINTER_TYPE && null_ptr_cst_p (op1))
> +      else if (code0 == POINTER_TYPE && null_ptr_cst_p (orig_op1))
>         {
>           result_type = type0;
>           if (extra_warnings && (complain & tf_warning))
>             warning (OPT_Wextra,
>                      "ordered comparison of pointer with integer zero");
>         }
> -      else if (code1 == POINTER_TYPE && null_ptr_cst_p (op0))
> +      else if (code1 == POINTER_TYPE && null_ptr_cst_p (orig_op0))
>         {
>           result_type = type1;
>           if (extra_warnings && (complain & tf_warning))
>             warning (OPT_Wextra,
>                      "ordered comparison of pointer with integer zero");
>         }
> -      else if (null_ptr_cst_p (op0) && null_ptr_cst_p (op1))
> +      else if (null_ptr_cst_p (orig_op0) && null_ptr_cst_p (orig_op1))
>         /* One of the operands must be of nullptr_t type.  */
>          result_type = TREE_TYPE (nullptr_node);
>        else if (code0 == POINTER_TYPE && code1 == INTEGER_TYPE)
> diff --git gcc/testsuite/g++.dg/cpp0x/nullptr37.C gcc/testsuite/g++.dg/cpp0x/nullptr37.C
> index e69de29..e746a28 100644
> --- gcc/testsuite/g++.dg/cpp0x/nullptr37.C
> +++ gcc/testsuite/g++.dg/cpp0x/nullptr37.C
> @@ -0,0 +1,78 @@
> +/* PR c++/64767 */
> +// { dg-do compile { target c++11 } }
> +
> +int
> +f1 (int *p, int **q)
> +{
> +  int r = 0;
> +
> +  r += p == '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += p == L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += p == u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += p == U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += p != '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += p != L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += p != u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += p != U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +
> +  r += '\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += L'\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += u'\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += U'\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += '\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += L'\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += u'\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += U'\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +
> +  r += q == '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += q == L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += q == u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += q == U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += q != '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += q != L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += q != u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += q != U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +
> +  r += '\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += L'\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += u'\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += U'\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += '\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += L'\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += u'\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += U'\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +
> +  return r;
> +}
> +
> +int
> +f2 (int *p)
> +{
> +  int r = 0;
> +
> +  r += p == (void *) 0;
> +  r += p != (void *) 0;
> +  r += (void *) 0 == p;
> +  r += (void *) 0 != p;
> +
> +  r += p == 0;
> +  r += p != 0;
> +  r += 0 == p;
> +  r += 0 != p;
> +
> +  return r;
> +}
> +
> +int
> +f3 (int *p)
> +{
> +  int r = 0;
> +
> +  r += p == (char) 0; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += p != (char) 0; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +
> +  r += (char) 0 == p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +  r += (char) 0 != p; // { dg-error "ISO C\\+\\+ forbids comparison between pointer and integer" }
> +
> +  return r;
> +}
>
>         Marek

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Eric Gallager]

On 10/2/16, Jason Merrill <jason@redhat.com> wrote:
> OK, thanks.
>
> On Sat, Oct 1, 2016 at 10:16 AM, Marek Polacek <polacek@redhat.com> wrote:
>> On Fri, Sep 30, 2016 at 05:48:03PM -0400, Jason Merrill wrote:
>>> On Fri, Sep 30, 2016 at 12:43 PM, Marek Polacek <polacek@redhat.com>
>>> wrote:
>>> > On Fri, Sep 23, 2016 at 10:31:33AM -0400, Jason Merrill wrote:
>>> >> On Fri, Sep 23, 2016 at 9:15 AM, Marek Polacek <polacek@redhat.com>
>>> >> wrote:
>>> >> > On Wed, Sep 21, 2016 at 03:52:09PM -0400, Jason Merrill wrote:
>>> >> >> On Mon, Sep 19, 2016 at 2:49 PM, Jason Merrill <jason@redhat.com>
>>> >> >> wrote:
>>> >> >> > I suppose that an INTEGER_CST of character type is necessarily a
>>> >> >> > character constant, so adding a check for !char_type_p ought to
>>> >> >> > do the
>>> >> >> > trick.
>>> >> >>
>>> >> >> Indeed it does.  I'm checking this in:
>>> >> >
>>> >> > Nice, thanks.  What about the original patch?  We still need to
>>> >> > warn
>>> >> > (or error for C++11) for pointer comparisons.
>>> >>
>>> >> If we still accept pointer comparisons in C++, that's another bug
>>> >> with
>>> >> treating \0 as a null pointer constant.  This seems to be because
>>> >> ocp_convert of \0 to int produces an INTEGER_CST indistinguishable
>>> >> from literal 0.
>>> >
>>> > I was trying to fix this in ocp_convert, by using NOP_EXPRs, but that
>>> > wasn't
>>> > successful.  But since we're interested in ==/!=, I think this can be
>>> > fixed
>>> > easily in cp_build_binary_op.  Actually, all that seems to be needed is
>>> > using
>>> > orig_op as the argument to null_ptr_cst_p, but that wouldn't give the
>>> > correct
>>> > diagnostics, so I did this.  By checking orig_op we can see if the
>>> > operands are
>>> > character literals or not, because orig_op is an operand before the
>>> > default
>>> > conversions.
>>>
>>> What is wrong about the diagnostic from just using orig_op?  "ISO C++
>>> forbids comparison between pointer and integer" seems fine to me, and
>>> will help the user to realize that they need to index off the pointer.
>>>
>>> I see that some of the calls to null_ptr_cst_p in cp_build_binary_op
>>> have already been changed to check orig_op*, but not all.  Let's
>>> update the remaining calls, that should do the trick without adding a
>>> new error.
>>
>> Here you go:
>>
>> Bootstrapped/regtested on x86_64-linux and ppc64-linux, ok for trunk?
>>
>> 2016-10-01  Marek Polacek  <polacek@redhat.com>
>>
>>         Core 903
>>         * typeck.c (cp_build_binary_op): Pass original operands to
>>         null_ptr_cst_p, not those after the default conversions.
>>
>>         * g++.dg/cpp0x/nullptr37.C: New test.
>>
>> diff --git gcc/cp/typeck.c gcc/cp/typeck.c
>> index 617ca55..8b780be 100644
>> --- gcc/cp/typeck.c
>> +++ gcc/cp/typeck.c
>> @@ -4573,7 +4573,7 @@ cp_build_binary_op (location_t location,
>>               || code1 == COMPLEX_TYPE || code1 == ENUMERAL_TYPE))
>>         short_compare = 1;
>>        else if (((code0 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type0))
>> -               && null_ptr_cst_p (op1))
>> +               && null_ptr_cst_p (orig_op1))
>>                /* Handle, eg, (void*)0 (c++/43906), and more.  */
>>                || (code0 == POINTER_TYPE
>>                    && TYPE_PTR_P (type1) && integer_zerop (op1)))
>> @@ -4587,7 +4587,7 @@ cp_build_binary_op (location_t location,
>>           warn_for_null_address (location, op0, complain);
>>         }
>>        else if (((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1))
>> -               && null_ptr_cst_p (op0))
>> +               && null_ptr_cst_p (orig_op0))
>>                /* Handle, eg, (void*)0 (c++/43906), and more.  */
>>                || (code1 == POINTER_TYPE
>>                    && TYPE_PTR_P (type0) && integer_zerop (op0)))
>> @@ -4604,7 +4604,7 @@ cp_build_binary_op (location_t location,
>>                || (TYPE_PTRDATAMEM_P (type0) && TYPE_PTRDATAMEM_P
>> (type1)))
>>         result_type = composite_pointer_type (type0, type1, op0, op1,
>>                                               CPO_COMPARISON, complain);
>> -      else if (null_ptr_cst_p (op0) && null_ptr_cst_p (op1))
>> +      else if (null_ptr_cst_p (orig_op0) && null_ptr_cst_p (orig_op1))
>>         /* One of the operands must be of nullptr_t type.  */
>>          result_type = TREE_TYPE (nullptr_node);
>>        else if (code0 == POINTER_TYPE && code1 == INTEGER_TYPE)
>> @@ -4623,7 +4623,7 @@ cp_build_binary_op (location_t location,
>>            else
>>              return error_mark_node;
>>         }
>> -      else if (TYPE_PTRMEMFUNC_P (type0) && null_ptr_cst_p (op1))
>> +      else if (TYPE_PTRMEMFUNC_P (type0) && null_ptr_cst_p (orig_op1))
>>         {
>>           if (TARGET_PTRMEMFUNC_VBIT_LOCATION
>>               == ptrmemfunc_vbit_in_delta)
>> @@ -4664,7 +4664,7 @@ cp_build_binary_op (location_t location,
>>             }
>>           result_type = TREE_TYPE (op0);
>>         }
>> -      else if (TYPE_PTRMEMFUNC_P (type1) && null_ptr_cst_p (op0))
>> +      else if (TYPE_PTRMEMFUNC_P (type1) && null_ptr_cst_p (orig_op0))
>>         return cp_build_binary_op (location, code, op1, op0, complain);
>>        else if (TYPE_PTRMEMFUNC_P (type0) && TYPE_PTRMEMFUNC_P (type1))
>>         {
>> @@ -4877,21 +4877,21 @@ cp_build_binary_op (location_t location,
>>        else if (code0 == POINTER_TYPE && code1 == POINTER_TYPE)
>>         result_type = composite_pointer_type (type0, type1, op0, op1,
>>                                               CPO_COMPARISON, complain);
>> -      else if (code0 == POINTER_TYPE && null_ptr_cst_p (op1))
>> +      else if (code0 == POINTER_TYPE && null_ptr_cst_p (orig_op1))
>>         {
>>           result_type = type0;
>>           if (extra_warnings && (complain & tf_warning))
>>             warning (OPT_Wextra,
>>                      "ordered comparison of pointer with integer zero");
>>         }
>> -      else if (code1 == POINTER_TYPE && null_ptr_cst_p (op0))
>> +      else if (code1 == POINTER_TYPE && null_ptr_cst_p (orig_op0))
>>         {
>>           result_type = type1;
>>           if (extra_warnings && (complain & tf_warning))
>>             warning (OPT_Wextra,
>>                      "ordered comparison of pointer with integer zero");
>>         }
>> -      else if (null_ptr_cst_p (op0) && null_ptr_cst_p (op1))
>> +      else if (null_ptr_cst_p (orig_op0) && null_ptr_cst_p (orig_op1))
>>         /* One of the operands must be of nullptr_t type.  */
>>          result_type = TREE_TYPE (nullptr_node);
>>        else if (code0 == POINTER_TYPE && code1 == INTEGER_TYPE)
>> diff --git gcc/testsuite/g++.dg/cpp0x/nullptr37.C
>> gcc/testsuite/g++.dg/cpp0x/nullptr37.C
>> index e69de29..e746a28 100644
>> --- gcc/testsuite/g++.dg/cpp0x/nullptr37.C
>> +++ gcc/testsuite/g++.dg/cpp0x/nullptr37.C
>> @@ -0,0 +1,78 @@
>> +/* PR c++/64767 */
>> +// { dg-do compile { target c++11 } }
>> +
>> +int
>> +f1 (int *p, int **q)
>> +{
>> +  int r = 0;
>> +
>> +  r += p == '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += p == L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += p == u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += p == U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += p != '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += p != L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += p != u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += p != U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +
>> +  r += '\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += L'\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += u'\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += U'\0' == p; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += '\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += L'\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += u'\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += U'\0' != p; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +
>> +  r += q == '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += q == L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += q == u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += q == U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += q != '\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += q != L'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += q != u'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += q != U'\0'; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +
>> +  r += '\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += L'\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += u'\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += U'\0' == q; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += '\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += L'\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += u'\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +  r += U'\0' != q; // { dg-error "ISO C\\+\\+ forbids comparison between
>> pointer and integer" }
>> +
>> +  return r;
>> +}
>> +
>> +int
>> +f2 (int *p)
>> +{
>> +  int r = 0;
>> +
>> +  r += p == (void *) 0;
>> +  r += p != (void *) 0;
>> +  r += (void *) 0 == p;
>> +  r += (void *) 0 != p;
>> +
>> +  r += p == 0;
>> +  r += p != 0;
>> +  r += 0 == p;
>> +  r += 0 != p;
>> +
>> +  return r;
>> +}
>> +
>> +int
>> +f3 (int *p)
>> +{
>> +  int r = 0;
>> +
>> +  r += p == (char) 0; // { dg-error "ISO C\\+\\+ forbids comparison
>> between pointer and integer" }
>> +  r += p != (char) 0; // { dg-error "ISO C\\+\\+ forbids comparison
>> between pointer and integer" }
>> +
>> +  r += (char) 0 == p; // { dg-error "ISO C\\+\\+ forbids comparison
>> between pointer and integer" }
>> +  r += (char) 0 != p; // { dg-error "ISO C\\+\\+ forbids comparison
>> between pointer and integer" }
>> +
>> +  return r;
>> +}
>>
>>         Marek
>

So I'm still kind of unclear as to what got committed as a result of
this thread. It seems like there's a new diagnostic for C++11, but
what about other language standards? Is there still going to be a
separate -Wpointer-compare flag usable in plain C? When I tried with
trunk from yesterday, it still didn't work:

$ /usr/local/bin/gcc -Wpointer-compare -c unexmacosx.c
gcc: error: unrecognized command line option ‘-Wpointer-compare’; did
you mean ‘-Wnonnull-compare’?
$

I hope a separate -Wpointer-compare flag can make it in in time for GCC 7.

Thanks,
Eric