diff mbox

[PATCHv3,resent] Add a warning for suspicious use of conditional expressions in boolean context

Message ID HE1PR0701MB216957FFECFC258EAB69C007E4F70@HE1PR0701MB2169.eurprd07.prod.outlook.com
State New
Headers show

Commit Message

Bernd Edlinger Sept. 20, 2016, 6:40 p.m. UTC 
On 09/20/16 16:51, Jason Merrill wrote:
> On Tue, Sep 20, 2016 at 10:11 AM, Bernd Edlinger

> <bernd.edlinger@hotmail.de> wrote:

>> On 09/20/16 13:29, Kyrill Tkachov wrote:

>>>

>>> arm bootstrap is now failing:

>>> $SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in

>>> boolean context [-Werror=int-in-bool-context]

>>>       : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \

>>>                              ~~~~~~~~~~~~~^~~~~~~~~~

>>> $SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro

>>> 'TARGET_ARM_FP'

>>>      if (TARGET_ARM_FP)

>>>

>>>

>>> The full definition of TARGET_ARM_FP is:

>>> #define TARGET_ARM_FP            \

>>>     (!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4        \

>>>               : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \

>>>                 : 0)

>>>

>>> We want it set to 0 when there's no FP but when FP is available we set

>>> it to a bitmask

>>> to suggest the level that is available. That seems like a legitimate use

>>> to me.

>>>

>>

>> Ok, I see, sorry for that.

>>

>> I think I will have to suppress the warning if the conditional is in

>> a macro somehow.

>

> from_macro_expansion_at will help with that.

>

> Though it seems to me that the issue here is more that (TARGET_FP16 ?

> 14 : 12) is not in a boolean context, it's in an integer context; only

> the outer ?: is in a boolean context.

>

> I also still think the warning message should be changed.

>


I try this:



That will fix "if (TARGET_ARM_FP)" which is fine.

But this seems to suppress all such warnings from an assert
macro too.  Like for instance "assert(a?1:2)".

Sure, we would not be interested in a ?: that is part of the assert
macro itself, but the expression that is evaluated by the macro should
be checked, but that is no longer done, because the macro parameter is
now also from the macro expansion.  But it is initially from the
macro invocation point.  Ideas?


Thanks
Bernd.

Comments

Bernd Edlinger Sept. 21, 2016, 3:43 p.m. UTC | #1 
On 09/21/16 17:00, Jason Merrill wrote:
> On 09/20/2016 02:40 PM, Bernd Edlinger wrote:
>> On 09/20/16 16:51, Jason Merrill wrote:
>>> On Tue, Sep 20, 2016 at 10:11 AM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> I think I will have to suppress the warning if the conditional is in
>>>> a macro somehow.
>>>
>>> from_macro_expansion_at will help with that.
>>>
>>> Though it seems to me that the issue here is more that (TARGET_FP16 ?
>>> 14 : 12) is not in a boolean context, it's in an integer context; only
>>> the outer ?: is in a boolean context.
>>>
>> +      if (warn_int_in_bool_context
>> +  && !from_macro_expansion_at (EXPR_LOCATION (expr)))
>>
>> But this seems to suppress all such warnings from an assert
>> macro too.  Like for instance "assert(a?1:2)".
>>
>> Sure, we would not be interested in a ?: that is part of the assert
>> macro itself, but the expression that is evaluated by the macro should
>> be checked, but that is no longer done, because the macro parameter is
>> now also from the macro expansion.  But it is initially from the
>> macro invocation point.  Ideas?
>
> This should fix that, though I haven't run regression tests yet:
>

Yes.  I think that goes in the right direction,
but it does not work yet.

#define XXX (a ? 2 : 3)

if (XXX) // prints a warning, but it should not.


Bernd.
diff mbox

Patch

Index: c-common.c
===================================================================
--- c-common.c	(revision 240268)
+++ c-common.c	(working copy)
@@ -4652,7 +4652,8 @@ 
  					       TREE_OPERAND (expr, 0));

      case COND_EXPR:
-      if (warn_int_in_bool_context)
+      if (warn_int_in_bool_context
+	  && !from_macro_expansion_at (EXPR_LOCATION (expr)))
  	{
  	  tree val1 = fold_for_warn (TREE_OPERAND (expr, 1));
  	  tree val2 = fold_for_warn (TREE_OPERAND (expr, 2));
@@ -4663,7 +4664,8 @@ 
  	      && (!integer_onep (val1)
  		  || !integer_onep (val2)))
  	    warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
-		        "?: using integer constants in boolean context");
+			"?: using integer constants in boolean context, "
+			"the expression will always evaluate to %<true%>");
  	}
        /* Distribute the conversion into the arms of a COND_EXPR.  */
        if (c_dialect_cxx ())