* [C++ Patch/RFC] PR 43906 @ 2014-08-04 16:24 Paolo Carlini 2014-08-04 20:45 ` Jason Merrill 0 siblings, 1 reply; 11+ messages in thread From: Paolo Carlini @ 2014-08-04 16:24 UTC (permalink / raw) To: gcc-patches; +Cc: Jason Merrill [-- Attachment #1: Type: text/plain, Size: 467 bytes --] Hi, I suppose we can quickly resolve, one way or another, this rather old issue. Considering: extern void z(); void h() { if ( z != (void*)0 ); } we -Waddress warn in C and we don't in C++, due to the rather subtle differences between null_pointer_constant_p and null_ptr_cst_p. I believe we could as well warn in C++ too, but then I'm afraid we have to handle the case specially, like in the below. What do you think? Thanks! Paolo. ////////////////////// [-- Attachment #2: patch_43906 --] [-- Type: text/plain, Size: 2442 bytes --] Index: cp/typeck.c =================================================================== --- cp/typeck.c (revision 213573) +++ cp/typeck.c (working copy) @@ -4353,12 +4353,11 @@ cp_build_binary_op (location_t location, && (code1 == INTEGER_TYPE || code1 == REAL_TYPE || code1 == COMPLEX_TYPE || code1 == ENUMERAL_TYPE)) short_compare = 1; - else if ((code0 == POINTER_TYPE && code1 == POINTER_TYPE) - || (TYPE_PTRDATAMEM_P (type0) && TYPE_PTRDATAMEM_P (type1))) - result_type = composite_pointer_type (type0, type1, op0, op1, - CPO_COMPARISON, complain); else if ((code0 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type0)) - && null_ptr_cst_p (op1)) + && (null_ptr_cst_p (op1) + /* Handle (void*)0 too. */ + || (TYPE_PTR_P (type1) && VOID_TYPE_P (TREE_TYPE (type1)) + && integer_zerop (op1)))) { if (TREE_CODE (op0) == ADDR_EXPR && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0))) @@ -4371,7 +4370,10 @@ cp_build_binary_op (location_t location, result_type = type0; } else if ((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1)) - && null_ptr_cst_p (op0)) + && (null_ptr_cst_p (op0) + /* Handle (void*)0 too. */ + || (TYPE_PTR_P (type0) && VOID_TYPE_P (TREE_TYPE (type0)) + && integer_zerop (op0)))) { if (TREE_CODE (op1) == ADDR_EXPR && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0))) @@ -4383,6 +4385,10 @@ cp_build_binary_op (location_t location, } result_type = type1; } + else if ((code0 == POINTER_TYPE && code1 == POINTER_TYPE) + || (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)) /* One of the operands must be of nullptr_t type. */ result_type = TREE_TYPE (nullptr_node); Index: testsuite/g++.dg/warn/Waddress-1.C =================================================================== --- testsuite/g++.dg/warn/Waddress-1.C (revision 0) +++ testsuite/g++.dg/warn/Waddress-1.C (working copy) @@ -0,0 +1,7 @@ +// PR c++/43906 +// { dg-options "-Waddress" } + +extern void z(); +void f() { if ( z ) z(); } // { dg-warning "address" } +void g() { if ( z != 0 ) z(); } // { dg-warning "address" } +void h() { if ( z != (void*)0 ) z(); } // { dg-warning "address" } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [C++ Patch/RFC] PR 43906 2014-08-04 16:24 [C++ Patch/RFC] PR 43906 Paolo Carlini @ 2014-08-04 20:45 ` Jason Merrill 2014-08-04 23:01 ` Paolo Carlini 0 siblings, 1 reply; 11+ messages in thread From: Jason Merrill @ 2014-08-04 20:45 UTC (permalink / raw) To: Paolo Carlini, gcc-patches On 08/04/2014 12:24 PM, Paolo Carlini wrote: > + || (TYPE_PTR_P (type1) && VOID_TYPE_P (TREE_TYPE (type1)) Why check for VOID_TYPE_P? I'd think we would want to warn about comparing to other null pointer values as well. Jason ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [C++ Patch/RFC] PR 43906 2014-08-04 20:45 ` Jason Merrill @ 2014-08-04 23:01 ` Paolo Carlini 2014-08-05 1:58 ` Jason Merrill 0 siblings, 1 reply; 11+ messages in thread From: Paolo Carlini @ 2014-08-04 23:01 UTC (permalink / raw) To: Jason Merrill, gcc-patches Hi, On 08/04/2014 10:45 PM, Jason Merrill wrote: > On 08/04/2014 12:24 PM, Paolo Carlini wrote: >> + || (TYPE_PTR_P (type1) && VOID_TYPE_P (TREE_TYPE (type1)) > > Why check for VOID_TYPE_P? I'd think we would want to warn about > comparing to other null pointer values as well. In fact I wondered about that a few minutes after sending my message... And this is what I figured out: normally we have hard errors from composite_pointer_type (eg, try scalar types, class types), even for null values. The only exception I have been able to find earlier today is that of pointer to the same function type, eg: extern void z(); typedef void (*ptr)(); void i() { if ( z != (ptr)0 ); } but in this case the C front-end too doesn't warn. In short, the case of (void*)0 seems very special. However, something I did *not* notice earlier today, is that comparing a pointer to function to a generic void* leads to a pedwarn at the beginning of composite_pointer_type about the comparison itself. Thus it's debatable whether we also want the -Waddress warning... If you ask me, closing the bug with a testcase which checks that we warn for -pedantic about the comparison (if we don't have one already) would be Ok (vs EDG 4.9 too) What do you think? Paolo. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [C++ Patch/RFC] PR 43906 2014-08-04 23:01 ` Paolo Carlini @ 2014-08-05 1:58 ` Jason Merrill 2014-08-05 11:49 ` Paolo Carlini 0 siblings, 1 reply; 11+ messages in thread From: Jason Merrill @ 2014-08-05 1:58 UTC (permalink / raw) To: Paolo Carlini, gcc-patches On 08/04/2014 07:01 PM, Paolo Carlini wrote: > In fact I wondered about that a few minutes after sending my message... > And this is what I figured out: normally we have hard errors from > composite_pointer_type (eg, try scalar types, class types), even for > null values. The only exception I have been able to find earlier today > is that of pointer to the same function type, eg: > > extern void z(); > typedef void (*ptr)(); > void i() { if ( z != (ptr)0 ); } > > but in this case the C front-end too doesn't warn. I don't see why we wouldn't want to warn in this case; it's still the case thet the comparison will always be false. We can also see this situation for non-function pointers: void f() { int i; if (&i != (int*)0); } Jason ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [C++ Patch/RFC] PR 43906 2014-08-05 1:58 ` Jason Merrill @ 2014-08-05 11:49 ` Paolo Carlini 2014-08-05 12:10 ` Paolo Carlini 0 siblings, 1 reply; 11+ messages in thread From: Paolo Carlini @ 2014-08-05 11:49 UTC (permalink / raw) To: Jason Merrill, gcc-patches [-- Attachment #1: Type: text/plain, Size: 1363 bytes --] Hi, On 08/05/2014 03:58 AM, Jason Merrill wrote: > On 08/04/2014 07:01 PM, Paolo Carlini wrote: >> In fact I wondered about that a few minutes after sending my message... >> And this is what I figured out: normally we have hard errors from >> composite_pointer_type (eg, try scalar types, class types), even for >> null values. The only exception I have been able to find earlier today >> is that of pointer to the same function type, eg: >> >> extern void z(); >> typedef void (*ptr)(); >> void i() { if ( z != (ptr)0 ); } >> >> but in this case the C front-end too doesn't warn. > I don't see why we wouldn't want to warn in this case; it's still the > case thet the comparison will always be false. In general, I agree of course. I was trying to understand if keeping the issue as minimally one of consistency with the C front-end simplified it. > We can also see this situation for non-function pointers: > > void f() > { > int i; > if (&i != (int*)0); > } Sure. Then, however, we must be careful about the actual pointer types, otherwise we change hard errors to warnings. And void* is an exception to the general rule. I tried using ptr_reasonably_similar to avoid the explicit call of comptypes + separate VOID_TYPE_P, but unfortunately it appears too loose about at least pointers to function types. Thanks! Paolo. ////////////////////////// [-- Attachment #2: patch_43906_3 --] [-- Type: text/plain, Size: 4330 bytes --] Index: cp/typeck.c =================================================================== --- cp/typeck.c (revision 213631) +++ cp/typeck.c (working copy) @@ -4353,12 +4353,14 @@ cp_build_binary_op (location_t location, && (code1 == INTEGER_TYPE || code1 == REAL_TYPE || code1 == COMPLEX_TYPE || code1 == ENUMERAL_TYPE)) short_compare = 1; - else if ((code0 == POINTER_TYPE && code1 == POINTER_TYPE) - || (TYPE_PTRDATAMEM_P (type0) && TYPE_PTRDATAMEM_P (type1))) - result_type = composite_pointer_type (type0, type1, op0, op1, - CPO_COMPARISON, complain); else if ((code0 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type0)) - && null_ptr_cst_p (op1)) + && (null_ptr_cst_p (op1) + /* Handle, eg, (void*)0 (c++/43906), and more. */ + || (TYPE_PTR_P (type1) && integer_zerop (op1) + && (VOID_TYPE_P (TREE_TYPE (type1)) + || comptypes (TYPE_MAIN_VARIANT (TREE_TYPE (type0)), + TYPE_MAIN_VARIANT (TREE_TYPE (type1)), + COMPARE_BASE | COMPARE_DERIVED))))) { if (TREE_CODE (op0) == ADDR_EXPR && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0))) @@ -4371,7 +4373,13 @@ cp_build_binary_op (location_t location, result_type = type0; } else if ((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1)) - && null_ptr_cst_p (op0)) + && (null_ptr_cst_p (op0) + /* Handle, eg, (void*)0 (c++/43906), and more. */ + || (TYPE_PTR_P (type0) && integer_zerop (op0) + && (VOID_TYPE_P (TREE_TYPE (type0)) + || comptypes (TYPE_MAIN_VARIANT (TREE_TYPE (type0)), + TYPE_MAIN_VARIANT (TREE_TYPE (type1)), + COMPARE_BASE | COMPARE_DERIVED))))) { if (TREE_CODE (op1) == ADDR_EXPR && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0))) @@ -4383,6 +4391,10 @@ cp_build_binary_op (location_t location, } result_type = type1; } + else if ((code0 == POINTER_TYPE && code1 == POINTER_TYPE) + || (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)) /* One of the operands must be of nullptr_t type. */ result_type = TREE_TYPE (nullptr_node); Index: testsuite/g++.dg/warn/Waddress-1.C =================================================================== --- testsuite/g++.dg/warn/Waddress-1.C (revision 0) +++ testsuite/g++.dg/warn/Waddress-1.C (working copy) @@ -0,0 +1,42 @@ +// PR c++/43906 +// { dg-options "-Waddress" } + +extern void z(); +typedef void (*ptrf) (); +typedef int (*ptrfn) (int); +int n; +const int m = 1; +struct S { }; +struct T : S { }; +struct U; +S s; +T t; +double d; + +void f() { if (z) z(); } // { dg-warning "address" } + +void gl() { if (z != 0) z(); } // { dg-warning "address" } +void hl() { if (z != (ptrf)0) z(); } // { dg-warning "address" } +void il() { if (z != (void*)0) z(); } // { dg-warning "address" } +void jl() { if (&n != (int*)0) z(); } // { dg-warning "address" } +void kl() { if (&m != (int*)0) z(); } // { dg-warning "address" } +void ll() { if (&s != (T*)0) z(); } // { dg-warning "address" } +void ml() { if (&t != (S*)0) z(); } // { dg-warning "address" } + +void nl() { if (z != (S*)0) z(); } // { dg-error "comparison" } +void pl() { if (z != (ptrfn)0) z(); } // { dg-error "comparison" } +void ql() { if (&d != (int*)0) z(); } // { dg-error "comparison" } +void rl() { if (&s != (U*)0) z(); } // { dg-error "comparison" } + +void gr() { if (0 != z) z(); } // { dg-warning "address" } +void hr() { if ((ptrf)0 != z) z(); } // { dg-warning "address" } +void ir() { if ((void*)0 != z) z(); } // { dg-warning "address" } +void jr() { if ((int*)0 != &n) z(); } // { dg-warning "address" } +void kr() { if ((int*)0 != &m) z(); } // { dg-warning "address" } +void lr() { if ((T*)0 != &s) z(); } // { dg-warning "address" } +void mr() { if ((S*)0 != &t) z(); } // { dg-warning "address" } + +void nr() { if ((S*)0 != z) z(); } // { dg-error "comparison" } +void pr() { if ((ptrfn)0 != z) z(); } // { dg-error "comparison" } +void qr() { if ((int*)0 != &d) z(); } // { dg-error "comparison" } +void rr() { if ((U*)0 != &s) z(); } // { dg-error "comparison" } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [C++ Patch/RFC] PR 43906 2014-08-05 11:49 ` Paolo Carlini @ 2014-08-05 12:10 ` Paolo Carlini 2014-08-05 12:33 ` Jason Merrill 0 siblings, 1 reply; 11+ messages in thread From: Paolo Carlini @ 2014-08-05 12:10 UTC (permalink / raw) To: Jason Merrill, gcc-patches .. a clarification. As I tried to briefly explain yesterday, this kind of change means that: extern void z(); void il() { if (z != (void*)0) z(); } doesn't trigger anymore the pedwarn at beginning of composite_pointer_type about the comparison itself, for the simple reason that we don't call it anymore. I suppose that's Ok. Otherwise we have to change something, it's a bit tricky tough. We could even decide that we don't want to handle the above for -Waddress because conceptually the issue with the comparison itself comes before, as again I tried to explain a bit yesterday, but that triggers a warning only with -pedantic (EDG has it by default). Paolo. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [C++ Patch/RFC] PR 43906 2014-08-05 12:10 ` Paolo Carlini @ 2014-08-05 12:33 ` Jason Merrill 2014-08-05 14:48 ` Paolo Carlini 0 siblings, 1 reply; 11+ messages in thread From: Jason Merrill @ 2014-08-05 12:33 UTC (permalink / raw) To: Paolo Carlini, gcc-patches On 08/05/2014 08:10 AM, Paolo Carlini wrote: > .. a clarification. As I tried to briefly explain yesterday, this kind > of change means that: > > extern void z(); > void il() { if (z != (void*)0) z(); } > > doesn't trigger anymore the pedwarn at beginning of > composite_pointer_type about the comparison itself, for the simple > reason that we don't call it anymore. So let's keep calling it when the RHS is also a pointer? Jason ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [C++ Patch/RFC] PR 43906 2014-08-05 12:33 ` Jason Merrill @ 2014-08-05 14:48 ` Paolo Carlini 2014-08-06 15:19 ` Jason Merrill 0 siblings, 1 reply; 11+ messages in thread From: Paolo Carlini @ 2014-08-05 14:48 UTC (permalink / raw) To: Jason Merrill, gcc-patches [-- Attachment #1: Type: text/plain, Size: 917 bytes --] Hi, On 08/05/2014 02:32 PM, Jason Merrill wrote: > On 08/05/2014 08:10 AM, Paolo Carlini wrote: >> .. a clarification. As I tried to briefly explain yesterday, this kind >> of change means that: >> >> extern void z(); >> void il() { if (z != (void*)0) z(); } >> >> doesn't trigger anymore the pedwarn at beginning of >> composite_pointer_type about the comparison itself, for the simple >> reason that we don't call it anymore. > So let's keep calling it when the RHS is also a pointer? Indeed ;) Then I'm finishing testing the below. Note: I also rearranged the conditionals, splitting out the TYPE_PTRDATAMEM_P case, which was causing confusion in some cases: we were feeding a TYPE_PTRDATAMEM_P and a TYPE_PTR_P to composite_pointer_type, thus obtaining immediately verbose diagnostic, instead of the expected clean one talking about invalid operands to operator!=. Thanks, Paolo. ///////////////////// [-- Attachment #2: patch_43906_4 --] [-- Type: text/plain, Size: 5129 bytes --] Index: cp/typeck.c =================================================================== --- cp/typeck.c (revision 213631) +++ cp/typeck.c (working copy) @@ -4353,13 +4353,22 @@ cp_build_binary_op (location_t location, && (code1 == INTEGER_TYPE || code1 == REAL_TYPE || code1 == COMPLEX_TYPE || code1 == ENUMERAL_TYPE)) short_compare = 1; - else if ((code0 == POINTER_TYPE && code1 == POINTER_TYPE) - || (TYPE_PTRDATAMEM_P (type0) && TYPE_PTRDATAMEM_P (type1))) - result_type = composite_pointer_type (type0, type1, op0, op1, - CPO_COMPARISON, complain); - else if ((code0 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type0)) - && null_ptr_cst_p (op1)) + else if (((code0 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type0)) + && null_ptr_cst_p (op1)) + /* Handle, eg, (void*)0 (c++/43906), and more. */ + || (code0 == POINTER_TYPE + && TYPE_PTR_P (type1) && integer_zerop (op1) + && (VOID_TYPE_P (TREE_TYPE (type1)) + || comptypes (TYPE_MAIN_VARIANT (TREE_TYPE (type0)), + TYPE_MAIN_VARIANT (TREE_TYPE (type1)), + COMPARE_BASE | COMPARE_DERIVED)))) { + if (TYPE_PTR_P (type1)) + result_type = composite_pointer_type (type0, type1, op0, op1, + CPO_COMPARISON, complain); + else + result_type = type0; + if (TREE_CODE (op0) == ADDR_EXPR && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0))) { @@ -4368,11 +4377,23 @@ cp_build_binary_op (location_t location, warning (OPT_Waddress, "the address of %qD will never be NULL", TREE_OPERAND (op0, 0)); } - result_type = type0; } - else if ((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1)) - && null_ptr_cst_p (op0)) + else if (((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1)) + && null_ptr_cst_p (op0)) + /* Handle, eg, (void*)0 (c++/43906), and more. */ + || (code1 == POINTER_TYPE + && TYPE_PTR_P (type0) && integer_zerop (op0) + && (VOID_TYPE_P (TREE_TYPE (type0)) + || comptypes (TYPE_MAIN_VARIANT (TREE_TYPE (type0)), + TYPE_MAIN_VARIANT (TREE_TYPE (type1)), + COMPARE_BASE | COMPARE_DERIVED)))) { + if (TYPE_PTR_P (type0)) + result_type = composite_pointer_type (type0, type1, op0, op1, + CPO_COMPARISON, complain); + else + result_type = type1; + if (TREE_CODE (op1) == ADDR_EXPR && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0))) { @@ -4381,8 +4402,11 @@ cp_build_binary_op (location_t location, warning (OPT_Waddress, "the address of %qD will never be NULL", TREE_OPERAND (op1, 0)); } - result_type = type1; } + else if ((code0 == POINTER_TYPE && code1 == POINTER_TYPE) + || (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)) /* One of the operands must be of nullptr_t type. */ result_type = TREE_TYPE (nullptr_node); Index: testsuite/g++.dg/warn/Waddress-1.C =================================================================== --- testsuite/g++.dg/warn/Waddress-1.C (revision 0) +++ testsuite/g++.dg/warn/Waddress-1.C (working copy) @@ -0,0 +1,42 @@ +// PR c++/43906 +// { dg-options "-Waddress -pedantic" } + +extern void z(); +typedef void (*ptrf) (); +typedef int (*ptrfn) (int); +int n; +const int m = 1; +struct S { }; +struct T : S { }; +struct U; +S s; +T t; +double d; + +void f() { if (z) z(); } // { dg-warning "address" } + +void gl() { if (z != 0) z(); } // { dg-warning "address" } +void hl() { if (z != (ptrf)0) z(); } // { dg-warning "address" } +void il() { if (z != (void*)0) z(); } // { dg-warning "address|comparison" } +void jl() { if (&n != (int*)0) z(); } // { dg-warning "address" } +void kl() { if (&m != (int*)0) z(); } // { dg-warning "address" } +void ll() { if (&s != (T*)0) z(); } // { dg-warning "address" } +void ml() { if (&t != (S*)0) z(); } // { dg-warning "address" } + +void nl() { if (z != (S*)0) z(); } // { dg-error "comparison" } +void pl() { if (z != (ptrfn)0) z(); } // { dg-error "comparison" } +void ql() { if (&d != (int*)0) z(); } // { dg-error "comparison" } +void rl() { if (&s != (U*)0) z(); } // { dg-error "comparison" } + +void gr() { if (0 != z) z(); } // { dg-warning "address" } +void hr() { if ((ptrf)0 != z) z(); } // { dg-warning "address" } +void ir() { if ((void*)0 != z) z(); } // { dg-warning "address|comparison" } +void jr() { if ((int*)0 != &n) z(); } // { dg-warning "address" } +void kr() { if ((int*)0 != &m) z(); } // { dg-warning "address" } +void lr() { if ((T*)0 != &s) z(); } // { dg-warning "address" } +void mr() { if ((S*)0 != &t) z(); } // { dg-warning "address" } + +void nr() { if ((S*)0 != z) z(); } // { dg-error "comparison" } +void pr() { if ((ptrfn)0 != z) z(); } // { dg-error "comparison" } +void qr() { if ((int*)0 != &d) z(); } // { dg-error "comparison" } +void rr() { if ((U*)0 != &s) z(); } // { dg-error "comparison" } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [C++ Patch/RFC] PR 43906 2014-08-05 14:48 ` Paolo Carlini @ 2014-08-06 15:19 ` Jason Merrill 2014-08-06 17:07 ` Paolo Carlini 0 siblings, 1 reply; 11+ messages in thread From: Jason Merrill @ 2014-08-06 15:19 UTC (permalink / raw) To: Paolo Carlini, gcc-patches On 08/05/2014 10:48 AM, Paolo Carlini wrote: > + && (VOID_TYPE_P (TREE_TYPE (type1)) > + || comptypes (TYPE_MAIN_VARIANT (TREE_TYPE (type0)), > + TYPE_MAIN_VARIANT (TREE_TYPE (type1)), > + COMPARE_BASE | COMPARE_DERIVED)))) Can we drop this now that we're calling composite_pointer_type? Jason ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [C++ Patch/RFC] PR 43906 2014-08-06 15:19 ` Jason Merrill @ 2014-08-06 17:07 ` Paolo Carlini 2014-08-06 18:20 ` Jason Merrill 0 siblings, 1 reply; 11+ messages in thread From: Paolo Carlini @ 2014-08-06 17:07 UTC (permalink / raw) To: Jason Merrill, gcc-patches [-- Attachment #1: Type: text/plain, Size: 1007 bytes --] Hi, On 08/06/2014 05:19 PM, Jason Merrill wrote: > On 08/05/2014 10:48 AM, Paolo Carlini wrote: >> + && (VOID_TYPE_P (TREE_TYPE (type1)) >> + || comptypes (TYPE_MAIN_VARIANT (TREE_TYPE (type0)), >> + TYPE_MAIN_VARIANT (TREE_TYPE (type1)), >> + COMPARE_BASE | COMPARE_DERIVED)))) > > Can we drop this now that we're calling composite_pointer_type? Yes we can, sorry for not investigating that earlier. I only have to tweak a bit the testcase because then in the malformed cases we emit first the permerror and then the -Waddress warning too. I suppose it's Ok because after all those are in most of the cases permerrors and I don't think the additional verbosity should be that common, we are talking about comparing a "null" pointer of the wrong type, not a generic pointer. Otherwise we would have to tweak composite_pointer_type to precisely inform the caller when an actual error was emitted. Thanks, Paolo. ///////////////////// [-- Attachment #2: patch_43906_5 --] [-- Type: text/plain, Size: 5151 bytes --] Index: cp/typeck.c =================================================================== --- cp/typeck.c (revision 213654) +++ cp/typeck.c (working copy) @@ -4353,13 +4353,18 @@ cp_build_binary_op (location_t location, && (code1 == INTEGER_TYPE || code1 == REAL_TYPE || code1 == COMPLEX_TYPE || code1 == ENUMERAL_TYPE)) short_compare = 1; - else if ((code0 == POINTER_TYPE && code1 == POINTER_TYPE) - || (TYPE_PTRDATAMEM_P (type0) && TYPE_PTRDATAMEM_P (type1))) - result_type = composite_pointer_type (type0, type1, op0, op1, - CPO_COMPARISON, complain); - else if ((code0 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type0)) - && null_ptr_cst_p (op1)) + else if (((code0 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type0)) + && null_ptr_cst_p (op1)) + /* Handle, eg, (void*)0 (c++/43906), and more. */ + || (code0 == POINTER_TYPE + && TYPE_PTR_P (type1) && integer_zerop (op1))) { + if (TYPE_PTR_P (type1)) + result_type = composite_pointer_type (type0, type1, op0, op1, + CPO_COMPARISON, complain); + else + result_type = type0; + if (TREE_CODE (op0) == ADDR_EXPR && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0))) { @@ -4368,11 +4373,19 @@ cp_build_binary_op (location_t location, warning (OPT_Waddress, "the address of %qD will never be NULL", TREE_OPERAND (op0, 0)); } - result_type = type0; } - else if ((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1)) - && null_ptr_cst_p (op0)) + else if (((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1)) + && null_ptr_cst_p (op0)) + /* Handle, eg, (void*)0 (c++/43906), and more. */ + || (code1 == POINTER_TYPE + && TYPE_PTR_P (type0) && integer_zerop (op0))) { + if (TYPE_PTR_P (type0)) + result_type = composite_pointer_type (type0, type1, op0, op1, + CPO_COMPARISON, complain); + else + result_type = type1; + if (TREE_CODE (op1) == ADDR_EXPR && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0))) { @@ -4381,8 +4394,11 @@ cp_build_binary_op (location_t location, warning (OPT_Waddress, "the address of %qD will never be NULL", TREE_OPERAND (op1, 0)); } - result_type = type1; } + else if ((code0 == POINTER_TYPE && code1 == POINTER_TYPE) + || (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)) /* One of the operands must be of nullptr_t type. */ result_type = TREE_TYPE (nullptr_node); Index: testsuite/g++.dg/warn/Waddress-1.C =================================================================== --- testsuite/g++.dg/warn/Waddress-1.C (revision 0) +++ testsuite/g++.dg/warn/Waddress-1.C (working copy) @@ -0,0 +1,50 @@ +// PR c++/43906 +// { dg-options "-Waddress -pedantic" } + +extern void z(); +typedef void (*ptrf) (); +typedef int (*ptrfn) (int); +int n; +const int m = 1; +struct S { }; +struct T : S { }; +struct U; +S s; +T t; +double d; + +void f() { if (z) z(); } // { dg-warning "address" } + +void gl() { if (z != 0) z(); } // { dg-warning "address" } +void hl() { if (z != (ptrf)0) z(); } // { dg-warning "address" } +void il() { if (z != (void*)0) z(); } // { dg-warning "address|comparison" } +void jl() { if (&n != (int*)0) z(); } // { dg-warning "address" } +void kl() { if (&m != (int*)0) z(); } // { dg-warning "address" } +void ll() { if (&s != (T*)0) z(); } // { dg-warning "address" } +void ml() { if (&t != (S*)0) z(); } // { dg-warning "address" } + +void nl() { if (z != (S*)0) z(); } // { dg-error "comparison" } +// { dg-warning "address" "" { target *-*-* } 26 } +void pl() { if (z != (ptrfn)0) z(); } // { dg-error "comparison" } +// { dg-warning "address" "" { target *-*-* } 28 } +void ql() { if (&d != (int*)0) z(); } // { dg-error "comparison" } +// { dg-warning "address" "" { target *-*-* } 30 } +void rl() { if (&s != (U*)0) z(); } // { dg-error "comparison" } +// { dg-warning "address" "" { target *-*-* } 32 } + +void gr() { if (0 != z) z(); } // { dg-warning "address" } +void hr() { if ((ptrf)0 != z) z(); } // { dg-warning "address" } +void ir() { if ((void*)0 != z) z(); } // { dg-warning "address|comparison" } +void jr() { if ((int*)0 != &n) z(); } // { dg-warning "address" } +void kr() { if ((int*)0 != &m) z(); } // { dg-warning "address" } +void lr() { if ((T*)0 != &s) z(); } // { dg-warning "address" } +void mr() { if ((S*)0 != &t) z(); } // { dg-warning "address" } + +void nr() { if ((S*)0 != z) z(); } // { dg-error "comparison" } +// { dg-warning "address" "" { target *-*-* } 43 } +void pr() { if ((ptrfn)0 != z) z(); } // { dg-error "comparison" } +// { dg-warning "address" "" { target *-*-* } 45 } +void qr() { if ((int*)0 != &d) z(); } // { dg-error "comparison" } +// { dg-warning "address" "" { target *-*-* } 47 } +void rr() { if ((U*)0 != &s) z(); } // { dg-error "comparison" } +// { dg-warning "address" "" { target *-*-* } 49 } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [C++ Patch/RFC] PR 43906 2014-08-06 17:07 ` Paolo Carlini @ 2014-08-06 18:20 ` Jason Merrill 0 siblings, 0 replies; 11+ messages in thread From: Jason Merrill @ 2014-08-06 18:20 UTC (permalink / raw) To: Paolo Carlini, gcc-patches OK, thanks. Jason ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-08-06 18:20 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-04 16:24 [C++ Patch/RFC] PR 43906 Paolo Carlini 2014-08-04 20:45 ` Jason Merrill 2014-08-04 23:01 ` Paolo Carlini 2014-08-05 1:58 ` Jason Merrill 2014-08-05 11:49 ` Paolo Carlini 2014-08-05 12:10 ` Paolo Carlini 2014-08-05 12:33 ` Jason Merrill 2014-08-05 14:48 ` Paolo Carlini 2014-08-06 15:19 ` Jason Merrill 2014-08-06 17:07 ` Paolo Carlini 2014-08-06 18:20 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).