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

[Kyrill Tkachov]

Hi Bernd,

On 19/09/16 22:32, Bernd Edlinger wrote:

On 09/19/16 21:51, Jeff Law wrote:

On 09/15/2016 03:19 AM, Bernd Edlinger wrote:

On 09/14/16 20:11, Jason Merrill wrote:

Yes.  The reasoning I initially had was that it is completely
pointless to have something of the form "if (x ? 1 : 2)" or
"if (x ? 0 : 0)" because the result does not even depend on x
in this case.  But something like "if (x ? 4999 : 0)" looks
bogus but does at least not ignore x.

If the false-positives are becoming too much of a problem here,
then I should of course revert to the previous heuristic again.

I think we could have both, where the weaker form is part of -Wall and
people can explicitly select the stronger form.

Yes, agreed.  So here is what I would think will be the first version.

It can later be extended to cover the more pedantic cases which
will not be enabled by -Wall.

I would like to send a follow-up patch for the warning on
signed-integer shift left in boolean context, which I think
should also be good for Wall.
(I already had that feature in patch version 2 but that's meanwhile
outdated).


Bootstrap and reg-testing on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.


changelog-pr77434v3.txt


gcc:
2016-09-14  Bernd Edlinger  <bernd.edlin...@hotmail.de>

     PR c++/77434
     * doc/invoke.texi: Document -Wcond-in-bool-context.

     PR middle-end/77421
     * dwarf2out.c (output_loc_operands): Fix an assertion.

c-family:
2016-09-14  Bernd Edlinger  <bernd.edlin...@hotmail.de>

     PR c++/77434
     * c.opt (Wcond-in-bool-context): New warning.
     * c-common.c (c_common_truthvalue_conversion): Warn on integer
     constants in boolean context.

cp:
2016-09-14  Bernd Edlinger  <bernd.edlin...@hotmail.de>

     PR c++/77434
     * cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here.

testsuite:
2016-09-14  Bernd Edlinger  <bernd.edlin...@hotmail.de>

     PR c++/77434
     * c-c++-common/Wcond-in-bool-context.c: New test.


patch-pr77434v3.diff


Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c    (revision 240135)
+++ gcc/builtins.c    (working copy)
@@ -7887,15 +7887,18 @@ fold_builtin_classify (location_t loc, tree
fndecl
          tree isinf_call = build_call_expr_loc (loc, isinf_fn, 1, arg);

          signbit_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
-                    signbit_call, integer_zero_node);
+                        signbit_call, integer_zero_node);
          isinf_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
-                      isinf_call, integer_zero_node);
+                      isinf_call, integer_zero_node);

-        tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
signbit_call,
-                   integer_minus_one_node, integer_one_node);
          tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
-                   isinf_call, tmp,
-                   integer_zero_node);
+                   signbit_call, integer_minus_one_node,
+                   integer_one_node);
+        /* Avoid a possible -Wint-in-bool-context warning in C.  */
+        tmp = fold_build2_loc (loc, PLUS_EXPR, integer_type_node, tmp,
+                   integer_zero_node);
+        tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
+                   isinf_call, tmp, integer_zero_node);
        }

      return tmp;

This hunk is not mentioned in the ChangeLog and there's a good chance
this has or is going to change significantly.  I don't like the tmp+0
workaround either.  If there isn't an immediate need, can we let this
hunk slide and come back to it after the other changes from Tamar &
Wilco are wrapped up?

Yes that would be good.

I think this is OK with the builtins.c hunk dropped as long as exclusion
of that hunk doesn't trigger spurious warnings.

It does trigger a warning but it is usually masked by -Wsystem-headers,
so I initially missed it completely, but it comes quite often when
I build a recent glibc.  That does only happen with C, not C++.

If I drop that chunk then I must also drop that line

    if (__builtin_isinf_sign ((float)a/b)) /* { dg-bogus "boolean
context" } */

But I have to come back and silence the warning on that construct
in a follow-up patch.


If it is OK, knowing it is work in progress,
I could hold back the builtins part for now,
and commit what I have ?

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.

Kyrill

Thanks
Bernd.