| Message ID | 20160316144552.GG10006@redhat.com |
|---|---|
| State | New |
| Headers | show |
OK. Jason
> @@ -3974,6 +3974,38 @@ build_vec_cmp (tree_code code, tree type, > return build3 (VEC_COND_EXPR, type, cmp, minus_one_vec, zero_vec); > } > > +/* Possibly warn about an address never being NULL. */ > + > +static void > +warn_for_null_address (location_t location, tree op, tsubst_flags_t complain) > +{ ... > + if (TREE_CODE (cop) == ADDR_EXPR > + && decl_with_nonnull_addr_p (TREE_OPERAND (cop, 0)) > + && !TREE_NO_WARNING (cop)) > + warning_at (location, OPT_Waddress, "the address of %qD will never " > + "be NULL", TREE_OPERAND (cop, 0)); > + > + if (CONVERT_EXPR_P (op) > + && TREE_CODE (TREE_TYPE (TREE_OPERAND (op, 0))) == REFERENCE_TYPE) > + { > + tree inner_op = op; > + STRIP_NOPS (inner_op); > + > + if (DECL_P (inner_op)) > + warning_at (location, OPT_Waddress, > + "the compiler can assume that the address of " > + "%qD will never be NULL", inner_op); Since I noted the subtle differences between the phrasing of the various -Waddress warnings in the bug, I have to ask: what is the significance of the difference between the two warnings here? Would it not be appropriate to issue the first warning in the latter case? Or perhaps even use the same text as is already used elsewhere: "the address of %qD will always evaluate as ‘true’" (since it may not be the macro NULL that's mentioned in the expression). Martin
On 03/16/2016 06:43 PM, Martin Sebor wrote: >> @@ -3974,6 +3974,38 @@ build_vec_cmp (tree_code code, tree type, >> return build3 (VEC_COND_EXPR, type, cmp, minus_one_vec, zero_vec); >> } >> >> +/* Possibly warn about an address never being NULL. */ >> + >> +static void >> +warn_for_null_address (location_t location, tree op, tsubst_flags_t >> complain) >> +{ > ... >> + if (TREE_CODE (cop) == ADDR_EXPR >> + && decl_with_nonnull_addr_p (TREE_OPERAND (cop, 0)) >> + && !TREE_NO_WARNING (cop)) >> + warning_at (location, OPT_Waddress, "the address of %qD will never " >> + "be NULL", TREE_OPERAND (cop, 0)); >> + >> + if (CONVERT_EXPR_P (op) >> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (op, 0))) == REFERENCE_TYPE) >> + { >> + tree inner_op = op; >> + STRIP_NOPS (inner_op); >> + >> + if (DECL_P (inner_op)) >> + warning_at (location, OPT_Waddress, >> + "the compiler can assume that the address of " >> + "%qD will never be NULL", inner_op); > > Since I noted the subtle differences between the phrasing of > the various -Waddress warnings in the bug, I have to ask: what is > the significance of the difference between the two warnings here? > > Would it not be appropriate to issue the first warning in the latter > case? Or perhaps even use the same text as is already used elsewhere: > "the address of %qD will always evaluate as ‘true’" (since it may not > be the macro NULL that's mentioned in the expression). They were added at different times AFAICT. The former is fairly old (Douglas Gregor, 2008) at this point. The latter was added by Patrick Palka for 65168 about a year ago. You could directly ask Patrick about motivations for a different message. Jeff
On Wed, Mar 16, 2016 at 06:43:39PM -0600, Martin Sebor wrote: > >@@ -3974,6 +3974,38 @@ build_vec_cmp (tree_code code, tree type, > > return build3 (VEC_COND_EXPR, type, cmp, minus_one_vec, zero_vec); > > } > > > >+/* Possibly warn about an address never being NULL. */ > >+ > >+static void > >+warn_for_null_address (location_t location, tree op, tsubst_flags_t complain) > >+{ > ... > >+ if (TREE_CODE (cop) == ADDR_EXPR > >+ && decl_with_nonnull_addr_p (TREE_OPERAND (cop, 0)) > >+ && !TREE_NO_WARNING (cop)) > >+ warning_at (location, OPT_Waddress, "the address of %qD will never " > >+ "be NULL", TREE_OPERAND (cop, 0)); > >+ > >+ if (CONVERT_EXPR_P (op) > >+ && TREE_CODE (TREE_TYPE (TREE_OPERAND (op, 0))) == REFERENCE_TYPE) > >+ { > >+ tree inner_op = op; > >+ STRIP_NOPS (inner_op); > >+ > >+ if (DECL_P (inner_op)) > >+ warning_at (location, OPT_Waddress, > >+ "the compiler can assume that the address of " > >+ "%qD will never be NULL", inner_op); > > Since I noted the subtle differences between the phrasing of > the various -Waddress warnings in the bug, I have to ask: what is > the significance of the difference between the two warnings here? Quite frankly, I don't know. > Would it not be appropriate to issue the first warning in the latter > case? Or perhaps even use the same text as is already used elsewhere: > "the address of %qD will always evaluate as ‘true’" (since it may not > be the macro NULL that's mentioned in the expression). There are more discrepancies in the front ends wrt error/warning messages. Perhaps we should try to unify them some more, but I don't think this has a big priority, if the message is clear enough for the users. Marek
On 03/16/2016 08:43 PM, Martin Sebor wrote: >> @@ -3974,6 +3974,38 @@ build_vec_cmp (tree_code code, tree type, >> return build3 (VEC_COND_EXPR, type, cmp, minus_one_vec, zero_vec); >> } >> >> +/* Possibly warn about an address never being NULL. */ >> + >> +static void >> +warn_for_null_address (location_t location, tree op, tsubst_flags_t >> complain) >> +{ > ... >> + if (TREE_CODE (cop) == ADDR_EXPR >> + && decl_with_nonnull_addr_p (TREE_OPERAND (cop, 0)) >> + && !TREE_NO_WARNING (cop)) >> + warning_at (location, OPT_Waddress, "the address of %qD will never " >> + "be NULL", TREE_OPERAND (cop, 0)); >> + >> + if (CONVERT_EXPR_P (op) >> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (op, 0))) == REFERENCE_TYPE) >> + { >> + tree inner_op = op; >> + STRIP_NOPS (inner_op); >> + >> + if (DECL_P (inner_op)) >> + warning_at (location, OPT_Waddress, >> + "the compiler can assume that the address of " >> + "%qD will never be NULL", inner_op); > > Since I noted the subtle differences between the phrasing of > the various -Waddress warnings in the bug, I have to ask: what is > the significance of the difference between the two warnings here? The difference is that in the second case, a reference could be bound to a null address, but that has undefined behavior, so the compiler can assume it won't happen. Jason
On 03/17/2016 10:45 AM, Jason Merrill wrote: > On 03/16/2016 08:43 PM, Martin Sebor wrote: >>> @@ -3974,6 +3974,38 @@ build_vec_cmp (tree_code code, tree type, >>> return build3 (VEC_COND_EXPR, type, cmp, minus_one_vec, zero_vec); >>> } >>> >>> +/* Possibly warn about an address never being NULL. */ >>> + >>> +static void >>> +warn_for_null_address (location_t location, tree op, tsubst_flags_t >>> complain) >>> +{ >> ... >>> + if (TREE_CODE (cop) == ADDR_EXPR >>> + && decl_with_nonnull_addr_p (TREE_OPERAND (cop, 0)) >>> + && !TREE_NO_WARNING (cop)) >>> + warning_at (location, OPT_Waddress, "the address of %qD will >>> never " >>> + "be NULL", TREE_OPERAND (cop, 0)); >>> + >>> + if (CONVERT_EXPR_P (op) >>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (op, 0))) == >>> REFERENCE_TYPE) >>> + { >>> + tree inner_op = op; >>> + STRIP_NOPS (inner_op); >>> + >>> + if (DECL_P (inner_op)) >>> + warning_at (location, OPT_Waddress, >>> + "the compiler can assume that the address of " >>> + "%qD will never be NULL", inner_op); >> >> Since I noted the subtle differences between the phrasing of >> the various -Waddress warnings in the bug, I have to ask: what is >> the significance of the difference between the two warnings here? > > The difference is that in the second case, a reference could be bound to > a null address, but that has undefined behavior, so the compiler can > assume it won't happen. So the first can't happen, the second could, but would be considered undefined behavior. jeff
On Thu, Mar 17, 2016 at 12:27 PM, Jeff Law <law@redhat.com> wrote: > On 03/16/2016 06:43 PM, Martin Sebor wrote: >>> >>> @@ -3974,6 +3974,38 @@ build_vec_cmp (tree_code code, tree type, >>> return build3 (VEC_COND_EXPR, type, cmp, minus_one_vec, zero_vec); >>> } >>> >>> +/* Possibly warn about an address never being NULL. */ >>> + >>> +static void >>> +warn_for_null_address (location_t location, tree op, tsubst_flags_t >>> complain) >>> +{ >> >> ... >>> >>> + if (TREE_CODE (cop) == ADDR_EXPR >>> + && decl_with_nonnull_addr_p (TREE_OPERAND (cop, 0)) >>> + && !TREE_NO_WARNING (cop)) >>> + warning_at (location, OPT_Waddress, "the address of %qD will never " >>> + "be NULL", TREE_OPERAND (cop, 0)); >>> + >>> + if (CONVERT_EXPR_P (op) >>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (op, 0))) == REFERENCE_TYPE) >>> + { >>> + tree inner_op = op; >>> + STRIP_NOPS (inner_op); >>> + >>> + if (DECL_P (inner_op)) >>> + warning_at (location, OPT_Waddress, >>> + "the compiler can assume that the address of " >>> + "%qD will never be NULL", inner_op); >> >> >> Since I noted the subtle differences between the phrasing of >> the various -Waddress warnings in the bug, I have to ask: what is >> the significance of the difference between the two warnings here? >> >> Would it not be appropriate to issue the first warning in the latter >> case? Or perhaps even use the same text as is already used elsewhere: >> "the address of %qD will always evaluate as ‘true’" (since it may not >> be the macro NULL that's mentioned in the expression). > > They were added at different times AFAICT. The former is fairly old > (Douglas Gregor, 2008) at this point. The latter was added by Patrick Palka > for 65168 about a year ago. > > You could directly ask Patrick about motivations for a different message. There is no plausible way for the address of a non-reference variable to be NULL even in code with UB (aside from __attribute__ ((weak)) in which case the warning is suppressed). But the address of a reference can easily seem to be NULL if one performs UB and assigns to it *(int *)NULL or something like that. I think that was my motivation, anyway :)
On 03/17/2016 10:48 AM, Patrick Palka wrote: > On Thu, Mar 17, 2016 at 12:27 PM, Jeff Law <law@redhat.com> wrote: >> On 03/16/2016 06:43 PM, Martin Sebor wrote: >>>> >>>> @@ -3974,6 +3974,38 @@ build_vec_cmp (tree_code code, tree type, >>>> return build3 (VEC_COND_EXPR, type, cmp, minus_one_vec, zero_vec); >>>> } >>>> >>>> +/* Possibly warn about an address never being NULL. */ >>>> + >>>> +static void >>>> +warn_for_null_address (location_t location, tree op, tsubst_flags_t >>>> complain) >>>> +{ >>> >>> ... >>>> >>>> + if (TREE_CODE (cop) == ADDR_EXPR >>>> + && decl_with_nonnull_addr_p (TREE_OPERAND (cop, 0)) >>>> + && !TREE_NO_WARNING (cop)) >>>> + warning_at (location, OPT_Waddress, "the address of %qD will never " >>>> + "be NULL", TREE_OPERAND (cop, 0)); >>>> + >>>> + if (CONVERT_EXPR_P (op) >>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (op, 0))) == REFERENCE_TYPE) >>>> + { >>>> + tree inner_op = op; >>>> + STRIP_NOPS (inner_op); >>>> + >>>> + if (DECL_P (inner_op)) >>>> + warning_at (location, OPT_Waddress, >>>> + "the compiler can assume that the address of " >>>> + "%qD will never be NULL", inner_op); >>> >>> >>> Since I noted the subtle differences between the phrasing of >>> the various -Waddress warnings in the bug, I have to ask: what is >>> the significance of the difference between the two warnings here? >>> >>> Would it not be appropriate to issue the first warning in the latter >>> case? Or perhaps even use the same text as is already used elsewhere: >>> "the address of %qD will always evaluate as ‘true’" (since it may not >>> be the macro NULL that's mentioned in the expression). >> >> They were added at different times AFAICT. The former is fairly old >> (Douglas Gregor, 2008) at this point. The latter was added by Patrick Palka >> for 65168 about a year ago. >> >> You could directly ask Patrick about motivations for a different message. > > There is no plausible way for the address of a non-reference variable > to be NULL even in code with UB (aside from __attribute__ ((weak)) in > which case the warning is suppressed). But the address of a reference > can easily seem to be NULL if one performs UB and assigns to it *(int > *)NULL or something like that. I think that was my motivation, anyway > :) Thanks (everyone) for the explanation. I actually think the warning Patrick added is the most accurate and would be appropriate in all cases. I suppose what bothers me besides the mention of NULL even when there is no NULL in the code, is that a) the text of the warnings is misleading (contradictory) in some interesting cases, and b) I can't think of a way in which the difference between the three phrasings of the diagnostic could be useful to a user. All three imply the same thing: compilers can and GCC is some cases does assume that the address of an ordinary (non weak) function, object, or reference is not null. To see (a), consider the invalid test program below, in which GCC makes this assumption about the address of i even though the warning doesn't mention it (but it makes a claim that's contrary to the actual address), yet doesn't make the same assumption about the address of the reference even though the diagnostic says it can. While I would find the warning less misleading if it simply said in all three cases: "the address of 'x' will always evaluate as ‘true’" I think it would be even more accurate if it said "the address of 'x' may be assumed to evaluate to ‘true’" That avoids making claims about whether or not it actually is null, doesn't talk about the NULL macro when one isn't used in the code, and by saying "may assume" it allows for both making the assumption as well as not making one. I'm happy to submit a patch to make this change in stage 1 if no one objects to it. Martin $ cat x.c && /home/msebor/build/gcc-trunk-svn/gcc/xgcc -B/home/msebor/build/gcc-trunk-svn/gcc -c -xc++ x.c && /home/msebor/build/gcc-trunk-svn/gcc/xgcc -B/home/msebor/build/gcc-trunk-svn/gcc -DMAIN -Wall -Wextra -Wpedantic x.o -xc++ x.c && ./a.out #if MAIN extern int i; extern int &r; extern void f (); int main () { f (); #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : "false") TEST (&i != 0); TEST (&r != 0); TEST (&i); } #else extern __attribute__ ((weak)) int i; int &r = i; void f () { __builtin_printf ("&i = %p\n&r = %p\n", &i, &r); } #endif x.c: In function ‘int main()’: x.c:14:17: warning: the address of ‘i’ will never be NULL [-Waddress] TEST (&i != 0); ^ x.c:12:54: note: in definition of macro ‘TEST’ #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : "false") ^ x.c:15:14: warning: the compiler can assume that the address of ‘r’ will never be NULL [-Waddress] TEST (&r != 0); ~~~^~~~ x.c:12:54: note: in definition of macro ‘TEST’ #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : "false") ^ x.c:12:68: warning: the address of ‘i’ will always evaluate as ‘true’ [-Waddres] #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : "false") ^ x.c:16:5: note: in expansion of macro ‘TEST’ TEST (&i); ^~~~ &i = (nil) &r = (nil) &i != 0 is true &r != 0 is false &i is true
On 03/17/2016 02:51 PM, Martin Sebor wrote: > On 03/17/2016 10:48 AM, Patrick Palka wrote: >> On Thu, Mar 17, 2016 at 12:27 PM, Jeff Law <law@redhat.com> wrote: >>> On 03/16/2016 06:43 PM, Martin Sebor wrote: >>>>> >>>>> @@ -3974,6 +3974,38 @@ build_vec_cmp (tree_code code, tree type, >>>>> return build3 (VEC_COND_EXPR, type, cmp, minus_one_vec, >>>>> zero_vec); >>>>> } >>>>> >>>>> +/* Possibly warn about an address never being NULL. */ >>>>> + >>>>> +static void >>>>> +warn_for_null_address (location_t location, tree op, tsubst_flags_t >>>>> complain) >>>>> +{ >>>> >>>> ... >>>>> >>>>> + if (TREE_CODE (cop) == ADDR_EXPR >>>>> + && decl_with_nonnull_addr_p (TREE_OPERAND (cop, 0)) >>>>> + && !TREE_NO_WARNING (cop)) >>>>> + warning_at (location, OPT_Waddress, "the address of %qD will >>>>> never " >>>>> + "be NULL", TREE_OPERAND (cop, 0)); >>>>> + >>>>> + if (CONVERT_EXPR_P (op) >>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (op, 0))) == >>>>> REFERENCE_TYPE) >>>>> + { >>>>> + tree inner_op = op; >>>>> + STRIP_NOPS (inner_op); >>>>> + >>>>> + if (DECL_P (inner_op)) >>>>> + warning_at (location, OPT_Waddress, >>>>> + "the compiler can assume that the address of " >>>>> + "%qD will never be NULL", inner_op); >>>> >>>> >>>> Since I noted the subtle differences between the phrasing of >>>> the various -Waddress warnings in the bug, I have to ask: what is >>>> the significance of the difference between the two warnings here? >>>> >>>> Would it not be appropriate to issue the first warning in the latter >>>> case? Or perhaps even use the same text as is already used elsewhere: >>>> "the address of %qD will always evaluate as ‘true’" (since it may not >>>> be the macro NULL that's mentioned in the expression). >>> >>> They were added at different times AFAICT. The former is fairly old >>> (Douglas Gregor, 2008) at this point. The latter was added by >>> Patrick Palka >>> for 65168 about a year ago. >>> >>> You could directly ask Patrick about motivations for a different >>> message. >> >> There is no plausible way for the address of a non-reference variable >> to be NULL even in code with UB (aside from __attribute__ ((weak)) in >> which case the warning is suppressed). But the address of a reference >> can easily seem to be NULL if one performs UB and assigns to it *(int >> *)NULL or something like that. I think that was my motivation, anyway >> :) > > Thanks (everyone) for the explanation. > > I actually think the warning Patrick added is the most accurate > and would be appropriate in all cases. > > I suppose what bothers me besides the mention of NULL even when > there is no NULL in the code, is that a) the text of the warnings > is misleading (contradictory) in some interesting cases, and b) > I can't think of a way in which the difference between the three > phrasings of the diagnostic could be useful to a user. All three > imply the same thing: compilers can and GCC is some cases does > assume that the address of an ordinary (non weak) function, object, > or reference is not null. > > To see (a), consider the invalid test program below, in which > GCC makes this assumption about the address of i even though > the warning doesn't mention it (but it makes a claim that's > contrary to the actual address), yet doesn't make the same > assumption about the address of the reference even though > the diagnostic says it can. > > While I would find the warning less misleading if it simply said > in all three cases: "the address of 'x' will always evaluate as > ‘true’" I think it would be even more accurate if it said > "the address of 'x' may be assumed to evaluate to ‘true’" That > avoids making claims about whether or not it actually is null, > doesn't talk about the NULL macro when one isn't used in the > code, and by saying "may assume" it allows for both making > the assumption as well as not making one. That sounds good except that talking about 'true' is wrong when there is an explicit comparison to a null pointer constant. I'd be fine with changing "NULL" to "null" or similar. > I'm happy to submit a patch to make this change in stage 1 if > no one objects to it. > > Martin > > $ cat x.c && /home/msebor/build/gcc-trunk-svn/gcc/xgcc > -B/home/msebor/build/gcc-trunk-svn/gcc -c -xc++ x.c && > /home/msebor/build/gcc-trunk-svn/gcc/xgcc > -B/home/msebor/build/gcc-trunk-svn/gcc -DMAIN -Wall -Wextra -Wpedantic > x.o -xc++ x.c && ./a.out > #if MAIN > > extern int i; > extern int &r; > > extern void f (); > > int main () > { > f (); > > #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : "false") > > TEST (&i != 0); > TEST (&r != 0); > TEST (&i); > } > > #else > extern __attribute__ ((weak)) int i; > int &r = i; > > void f () > { > __builtin_printf ("&i = %p\n&r = %p\n", &i, &r); > } > > #endif > x.c: In function ‘int main()’: > x.c:14:17: warning: the address of ‘i’ will never be NULL [-Waddress] > TEST (&i != 0); > ^ > x.c:12:54: note: in definition of macro ‘TEST’ > #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : > "false") > ^ > x.c:15:14: warning: the compiler can assume that the address of ‘r’ will > never be NULL [-Waddress] > TEST (&r != 0); > ~~~^~~~ > x.c:12:54: note: in definition of macro ‘TEST’ > #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : > "false") > ^ > x.c:12:68: warning: the address of ‘i’ will always evaluate as ‘true’ > [-Waddres] > #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : > "false") > ^ > x.c:16:5: note: in expansion of macro ‘TEST’ > TEST (&i); > ^~~~ > &i = (nil) > &r = (nil) > &i != 0 is true > &r != 0 is false > &i is true >
>> While I would find the warning less misleading if it simply said >> in all three cases: "the address of 'x' will always evaluate as >> ‘true’" I think it would be even more accurate if it said >> "the address of 'x' may be assumed to evaluate to ‘true’" That >> avoids making claims about whether or not it actually is null, >> doesn't talk about the NULL macro when one isn't used in the >> code, and by saying "may assume" it allows for both making >> the assumption as well as not making one. > > That sounds good except that talking about 'true' is wrong when there is > an explicit comparison to a null pointer constant. I'd be fine with > changing "NULL" to "null" or similar. Sounds good. I will use bug 47931 - missing -Waddress warning for comparison with NULL, to take care of the outstanding cases where a warning still isn't issued (in either C++ or C) and also adjust the text of the warning. Martin PS It seems that just adding STRIP_NOPS (op) to Marek's patch significantly increases the number of successfully diagnosed cases. (The small patch I attached to 47931 covers nearly all the remaining cases I could think of.)
diff --git gcc/cp/typeck.c gcc/cp/typeck.c index 20f0afc..447006c 100644 --- gcc/cp/typeck.c +++ gcc/cp/typeck.c @@ -3974,6 +3974,38 @@ build_vec_cmp (tree_code code, tree type, return build3 (VEC_COND_EXPR, type, cmp, minus_one_vec, zero_vec); } +/* Possibly warn about an address never being NULL. */ + +static void +warn_for_null_address (location_t location, tree op, tsubst_flags_t complain) +{ + if (!warn_address + || (complain & tf_warning) == 0 + || c_inhibit_evaluation_warnings != 0 + || TREE_NO_WARNING (op)) + return; + + tree cop = fold_non_dependent_expr (op); + + if (TREE_CODE (cop) == ADDR_EXPR + && decl_with_nonnull_addr_p (TREE_OPERAND (cop, 0)) + && !TREE_NO_WARNING (cop)) + warning_at (location, OPT_Waddress, "the address of %qD will never " + "be NULL", TREE_OPERAND (cop, 0)); + + if (CONVERT_EXPR_P (op) + && TREE_CODE (TREE_TYPE (TREE_OPERAND (op, 0))) == REFERENCE_TYPE) + { + tree inner_op = op; + STRIP_NOPS (inner_op); + + if (DECL_P (inner_op)) + warning_at (location, OPT_Waddress, + "the compiler can assume that the address of " + "%qD will never be NULL", inner_op); + } +} + /* Build a binary-operation expression without default conversions. CODE is the kind of expression to build. LOCATION is the location_t of the operator in the source code. @@ -4520,32 +4552,7 @@ cp_build_binary_op (location_t location, else result_type = type0; - if (TREE_CODE (op0) == ADDR_EXPR - && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0))) - { - if ((complain & tf_warning) - && c_inhibit_evaluation_warnings == 0 - && !TREE_NO_WARNING (op0)) - warning (OPT_Waddress, "the address of %qD will never be NULL", - TREE_OPERAND (op0, 0)); - } - - if (CONVERT_EXPR_P (op0) - && TREE_CODE (TREE_TYPE (TREE_OPERAND (op0, 0))) - == REFERENCE_TYPE) - { - tree inner_op0 = op0; - STRIP_NOPS (inner_op0); - - if ((complain & tf_warning) - && c_inhibit_evaluation_warnings == 0 - && !TREE_NO_WARNING (op0) - && DECL_P (inner_op0)) - warning_at (location, OPT_Waddress, - "the compiler can assume that the address of " - "%qD will never be NULL", - inner_op0); - } + warn_for_null_address (location, op0, complain); } else if (((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1)) && null_ptr_cst_p (op0)) @@ -4559,32 +4566,7 @@ cp_build_binary_op (location_t location, else result_type = type1; - if (TREE_CODE (op1) == ADDR_EXPR - && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0))) - { - if ((complain & tf_warning) - && c_inhibit_evaluation_warnings == 0 - && !TREE_NO_WARNING (op1)) - warning (OPT_Waddress, "the address of %qD will never be NULL", - TREE_OPERAND (op1, 0)); - } - - if (CONVERT_EXPR_P (op1) - && TREE_CODE (TREE_TYPE (TREE_OPERAND (op1, 0))) - == REFERENCE_TYPE) - { - tree inner_op1 = op1; - STRIP_NOPS (inner_op1); - - if ((complain & tf_warning) - && c_inhibit_evaluation_warnings == 0 - && !TREE_NO_WARNING (op1) - && DECL_P (inner_op1)) - warning_at (location, OPT_Waddress, - "the compiler can assume that the address of " - "%qD will never be NULL", - inner_op1); - } + warn_for_null_address (location, op1, complain); } else if ((code0 == POINTER_TYPE && code1 == POINTER_TYPE) || (TYPE_PTRDATAMEM_P (type0) && TYPE_PTRDATAMEM_P (type1))) diff --git gcc/testsuite/g++.dg/warn/constexpr-70194.C gcc/testsuite/g++.dg/warn/constexpr-70194.C index e69de29..cdc56c0 100644 --- gcc/testsuite/g++.dg/warn/constexpr-70194.C +++ gcc/testsuite/g++.dg/warn/constexpr-70194.C @@ -0,0 +1,12 @@ +// PR c++/70194 +// { dg-do compile { target c++11 } } +// { dg-options "-Wall" } + +int i; + +const bool b0 = &i == 0; // { dg-warning "the address of .i. will never be NULL" } +constexpr int *p = &i; +const bool b1 = p == 0; // { dg-warning "the address of .i. will never be NULL" } +const bool b2 = 0 == p; // { dg-warning "the address of .i. will never be NULL" } +const bool b3 = p != 0; // { dg-warning "the address of .i. will never be NULL" } +const bool b4 = 0 != p; // { dg-warning "the address of .i. will never be NULL" }
On Tue, Mar 15, 2016 at 03:41:52PM -0400, Jason Merrill wrote: > Let's factor out that duplicated code into a separate function. Sure. It also allowed me to hoist the cheap tests for both warnings, and while at it, I used 'location' for the first warning. Bootstrapped/regtested on x86_64-linux, ok for trunk? 2016-03-16 Marek Polacek <polacek@redhat.com> PR c++/70194 * typeck.c (warn_for_null_address): New function. (cp_build_binary_op): Call it. * g++.dg/warn/constexpr-70194.C: New test. Marek