Dirk Mueller - [PATCH] Implement warning for wrong use of ||/&& (original) (raw)

This is the mail archive of the gcc-patches@gcc.gnu.orgmailing list for the GCC project.

• From: Dirk Mueller
• To: gcc-patches at gcc dot gnu dot org
• Date: Tue, 13 Feb 2007 00:09:05 +0100
• Subject: [PATCH] Implement warning for wrong use of ||/&&

Hi,

the patch below implements a warning for apparently wrong usage of "||" and "&&" in contexts where likely a "|" or "&" was intended by the programmer. This seems very low false-positive to me, and it has found dozens of real faults in real world code (actually all code I tried it on, including gcc itself). I was actually trying to submit patches for those all by myself to get all the honor, but on the other hand I'm too lazy for that so I'd rather like to get the warning into gcc so that everyone can fix their code instead ;)

I've spent several hours trying to come up for a good name for this warning, but couldn't. so I've plumbed it into -Walways-true. Matches perfectly IMHO.

bootstrapped, regtested against i686-suse-linux, no regressions. Ok?

Thanks, Dirk

2007-02-13 Dirk Mueller dmueller@suse.de

    * c-typeck.c (parser_build_binary_op): Warn about expressions
    of &&/|| and non-zero constants.

    * cp/call.c (build_new_op): Warn about expressions
    of &&/|| and non-zero constants.

    * gcc.dg/Walways-true-3.c: New testcase.
    * g++.dg/warn/Walways-true-3.C: New testcase.

--- c-typeck.c (revision 121595) +++ c-typeck.c (working copy) @@ -2634,6 +2634,33 @@ parser_build_binary_op (enum tree_code c if (warn_parentheses) warn_about_parentheses (code, code1, code2); + switch (code) + { + case TRUTH_ANDIF_EXPR: + case TRUTH_ORIF_EXPR: + case TRUTH_OR_EXPR: + case TRUTH_AND_EXPR: + if (!TREE_NO_WARNING (arg1.value) + && INTEGRAL_TYPE_P (TREE_TYPE (arg1.value)) + && !CONSTANT_CLASS_P (arg1.value) + && TREE_CODE_CLASS (code1) != tcc_comparison + && TREE_CODE (arg2.value) == INTEGER_CST + && !integer_zerop (arg2.value) + && !integer_onep (arg2.value)) + { + warning (OPT_Walways_true, + "logical %<%s%> with non-zero constant " + "will always evaluate as true", + ((code == TRUTH_ANDIF_EXPR) + || (code == TRUTH_AND_EXPR)) ? "&&" : "||"); + TREE_NO_WARNING (arg1.value) = 1; + } + + break; + default: + break; + } + /* Warn about comparisons against string literals, with the exception of testing for equality or inequality of a string literal with NULL. */ if (code == EQ_EXPR || code == NE_EXPR) --- cp/call.c (revision 121595) +++ cp/call.c (working copy) @@ -3665,6 +3680,8 @@ build_new_op (enum tree_code code, int f void *p; bool strict_p; bool any_viable_p; + bool expl_eq_arg1 = false; + bool expl_eq_arg2 = false; if (error_operand_p (arg1) || error_operand_p (arg2) @@ -3694,6 +3711,14 @@ build_new_op (enum tree_code code, int f case CALL_EXPR: return build_object_call (arg1, arg2); + case TRUTH_ORIF_EXPR: + case TRUTH_ANDIF_EXPR: + case TRUTH_AND_EXPR: + case TRUTH_OR_EXPR: + if (COMPARISON_CLASS_P (arg1)) + expl_eq_arg1 = true; + if (COMPARISON_CLASS_P (arg2)) + expl_eq_arg2 = true; default: break; } @@ -3901,6 +3926,28 @@ build_new_op (enum tree_code code, int f conv = conv->u.next; arg3 = convert_like (conv, arg3); } + + switch (code) + { + case TRUTH_ANDIF_EXPR: + case TRUTH_ORIF_EXPR: + case TRUTH_AND_EXPR: + case TRUTH_OR_EXPR: + if (INTEGRAL_TYPE_P (TREE_TYPE (arg1)) + && !CONSTANT_CLASS_P (arg1) + && !expl_eq_arg1 + && TREE_CODE (arg2) == INTEGER_CST + && !integer_zerop (arg2)) + { + warning (OPT_Walways_true, "logical %<%s%> with non-zero constant " + "will always evaluate as true", + ((code == TRUTH_ANDIF_EXPR) + || (code == TRUTH_AND_EXPR)) ? "&&" : "||"); + expl_eq_arg1 = expl_eq_arg2 = true; + } + default: + break; + } } } @@ -3921,6 +3968,18 @@ build_new_op (enum tree_code code, int f case INDIRECT_REF: return build_indirect_ref (arg1, "unary *"); + case TRUTH_ANDIF_EXPR: + case TRUTH_ORIF_EXPR: + if (INTEGRAL_TYPE_P (TREE_TYPE (arg1)) + && !CONSTANT_CLASS_P (arg1) + && !expl_eq_arg1 + && TREE_CODE (arg2) == INTEGER_CST + && !integer_zerop (arg2) + && !integer_onep (arg2)) + warning (OPT_Walways_true, "logical %<%s%> with non-zero constant " + "will always evalue as true", + ((code == TRUTH_ANDIF_EXPR) + || (code == TRUTH_AND_EXPR)) ? "&&" : "||"); case PLUS_EXPR: case MINUS_EXPR: case MULT_EXPR: @@ -3939,8 +3998,6 @@ build_new_op (enum tree_code code, int f case BIT_AND_EXPR: case BIT_IOR_EXPR: case BIT_XOR_EXPR: - case TRUTH_ANDIF_EXPR: - case TRUTH_ORIF_EXPR: return cp_build_binary_op (code, arg1, arg2); case UNARY_PLUS_EXPR: Index: testsuite/gcc.dg/Walways-true-3.c

--- testsuite/gcc.dg/Walways-true-3.c (revision 0) +++ testsuite/gcc.dg/Walways-true-3.c (revision 0) @@ -0,0 +1,47 @@ +/* + { dg-do compile} + { dg-options "-Walways-true" } +/ + +enum { a, ba, b }; + +enum testenum { t1, t2}; + +extern int c; +extern char bool_a, bool_b; + +extern int testa(); + +void foo() +{ + if ( testa() && b ) / { dg-warning "always evaluate as" } / + (void)testa(); + + if ( c && b ) / { dg-warning "always evaluate as" } / + (void)testa(); + + if ( c && 0x42 ) / { dg-warning "always evaluate as" } / + (void)testa(); + + if ( c && 0x42 ) / { dg-warning "always evaluate as" } / + (void) testa(); + + if ( c && 0x80 >>6) / { dg-warning "always evaluate as" } / + (void)testa(); + + + if ( b && c == a ) / { dg-bogus "always evaluate as" } / + (void)testa(); + + if ( 1 && c ) / { dg-warning "always evaluate as" } / + (void)testa(); + + if ( t2 && b ) / { dg-warning "always evaluate as" } / + (void)testa(); + + if ( 0 && c == a ) / { dg-warning "always evaluate as" } / + (void)testa(); + + if ( b && 1 ) / { dg-warning "always evaluate as" } */ + (void)testa(); +} Index: testsuite/g++.dg/warn/Walways-true-3.C

--- testsuite/g++.dg/warn/Walways-true-3.C (revision 0) +++ testsuite/g++.dg/warn/Walways-true-3.C (revision 0) @@ -0,0 +1,46 @@ +// { dg-do compile} +// { dg-options "-Walways-true" } + +enum { a, b }; + +enum testenum { t1, t2}; + +extern int c; +extern bool bool_a, bool_b; + +template +class QFlags +{ +public:

• typedef void **Zero;
• int i;
• inline QFlags(Enum f) : i(f) {}
• inline operator int() const
• { return i;}
• +};
• +QFlags f(t2); +extern void do_something(int);
• +extern testenum testa();
• +void foo() +{
• if ( f && b ) // { dg-warning "always evaluate as" }

•      do_something(1);

• if ( c && b ) // { dg-warning "always evaluate as" }

•      do_something(2);

• if ( b && c == a ) // { dg-bogus "always evaluate as" }

•      do_something(101);

• if ( 1 && c )

•      do_something(102);  // { dg-bogus "always evaluate as" }

• if ( t2 && b ) // { dg-bogus "always evaluate as" }

•      do_something(103);

• if ( true && c == a ) // { dg-bogus "always evaluate as" }

•      do_something(104);

• if ( b && true ) // { dg-bogus "always evaluate as" }

•      do_something(105);

+}

• Follow-Ups:
  • Re: [PATCH] Implement warning for wrong use of ||/&&
    * From: Manuel López-Ibáñez
  • Re: [PATCH] Implement warning for wrong use of ||/&&
    * From: Manuel López-Ibáñez