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

[Joseph Myers]

On Wed, 4 Jan 2017, Marek Polacek wrote:

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

The C changes are OK.

-- 
Joseph S. Myers
jos...@codesourcery.com

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

[Jason Merrill]

On Wed, Jan 4, 2017 at 1:19 PM, Marek Polacek  wrote:
> +Note that the code above is illegal in C++11.

"invalid".  The C++ changes are OK.

Jason

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

[Eric Gallager]

On 10/2/16, Jason Merrill  wrote:
> OK, thanks.
>
> On Sat, Oct 1, 2016 at 10:16 AM, Marek Polacek  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 
>>> 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 
>>> >> 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 
>>> >> >> 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  
>>
>> 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) && 

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  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  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  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  
>> >> >> 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  
>
> 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 

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  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  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  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  

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,
   

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  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  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  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 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 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)

[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)

[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  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  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  

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 

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  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  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 Wed, Sep 21, 2016 at 03:52:09PM -0400, Jason Merrill wrote:
> On Mon, Sep 19, 2016 at 2:49 PM, Jason Merrill  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 Mon, Sep 19, 2016 at 2:49 PM, Jason Merrill  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:
commit d2f237ef0f63b3ee3da79bcbfad08fedb325d554
Author: Jason Merrill 
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 000..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)

[Jason Merrill]

On Thu, Sep 15, 2016 at 8:16 AM, Marek Polacek  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  wrote:
>> > Spurred by the recent  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)

[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  wrote:
> > Spurred by the recent  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 Sat, Sep 10, 2016 at 10:58 AM, Marek Polacek  wrote:
> Spurred by the recent  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 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  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  

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 (, 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 

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  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