[PATCH] PR 17946: warning message for suspect "a && MASK"

[Thomas R. Truscott]

People sometimes code "a && MASK" when they intended "a & MASK".
gcc itself does not seem to have examples of this,
here are some in the linux-2.4.20 kernel:

drivers/scsi/cpqfcTSworker.c
          printk(" Got 3 LOGOuts - terminating comm. with port_id %Xh\n",       
			  fchs->s_id &&0xFFFFFF);

drivers/usb/serial/pl2303.c
	if (cflag && CBAUD) {

drivers/tc/lk201.c
		handle_scancode(c, shift_state && LK_LOCK ? 1 : 0);
		handle_scancode(c, shift_state && LK_SHIFT ? 1 : 0);
                ...

Here is a patch that warns about A && B when

   A and B are both integer type (e.g. neither pointer nor real)
   A or B, but not both, is an integer constant
   Neither A nor B are (appear to be) limited to 0/1 (boolean).

I've used this warning for years with excellent results.
One might expect lots of false positives, but I cannot recall even one.

There is an ugly aspect to the patch.
c-parse.in converts A to truth value (A != 0) to update "skip_evaluation".
But the unconverted A is needed for this warning check.
To get around this, the patch does a temporary conversion. Twice. Yuck.
Perhaps there is a better way.


2004-10-08 Tom Truscott <trt@acm.org>

	PR 17946
	* c-parse.in: Pass unconverted lhs to parse_build_binary_op.
	* c-typeck.c (build_binary_op): Add new warning.
	* doc/invoke.texi: Document new warning.
	* testsuite/gcc.dg/wandand.c: New test.

Index: gcc/gcc/c-parse.in
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/c-parse.in,v
retrieving revision 1.245
diff -c -3 -p -r1.245 c-parse.in
*** gcc/gcc/c-parse.in	6 Oct 2004 04:47:26 -0000	1.245
--- gcc/gcc/c-parse.in	8 Oct 2004 14:15:48 -0000
*************** expr_no_commas:
*** 610,620 ****
  	| expr_no_commas '^' expr_no_commas
  		{ $$ = parser_build_binary_op ($2, $1, $3); }
  	| expr_no_commas ANDAND
! 		{ $1.value = lang_hooks.truthvalue_conversion
  		    (default_conversion ($1.value));
! 		  skip_evaluation += $1.value == truthvalue_false_node; }
  	  expr_no_commas
! 		{ skip_evaluation -= $1.value == truthvalue_false_node;
  		  $$ = parser_build_binary_op (TRUTH_ANDIF_EXPR, $1, $4); }
  	| expr_no_commas OROR
  		{ $1.value = lang_hooks.truthvalue_conversion
--- 610,622 ----
  	| expr_no_commas '^' expr_no_commas
  		{ $$ = parser_build_binary_op ($2, $1, $3); }
  	| expr_no_commas ANDAND
! 		{ tree t = lang_hooks.truthvalue_conversion
  		    (default_conversion ($1.value));
! 		  skip_evaluation += t == truthvalue_false_node; }
  	  expr_no_commas
! 		{ tree t = lang_hooks.truthvalue_conversion
! 		    (default_conversion ($1.value));
! 		  skip_evaluation -= t == truthvalue_false_node;
  		  $$ = parser_build_binary_op (TRUTH_ANDIF_EXPR, $1, $4); }
  	| expr_no_commas OROR
  		{ $1.value = lang_hooks.truthvalue_conversion

Index: gcc/gcc/c-typeck.c
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/c-typeck.c,v
retrieving revision 1.386
diff -c -3 -p -r1.386 c-typeck.c
*** gcc/gcc/c-typeck.c	6 Oct 2004 22:16:26 -0000	1.386
--- gcc/gcc/c-typeck.c	8 Oct 2004 14:15:48 -0000
*************** build_binary_op (enum tree_code code, tr
*** 7154,7159 ****
--- 7154,7168 ----
        break;
  
      case TRUTH_ANDIF_EXPR:
+       if (extra_warnings
+           && code0 == INTEGER_TYPE && code1 == INTEGER_TYPE
+           && CONSTANT_CLASS_P (op0) != CONSTANT_CLASS_P (op1)
+           && !COMPARISON_CLASS_P (op0) && !integer_zerop (op0) && !integer_onep (op0)
+           && !COMPARISON_CLASS_P (op1) && !integer_zerop (op1) && !integer_onep (op1))
+         warning ("non-boolean constant in boolean context");
+ 
+       /* FALLTHRU */
+ 
      case TRUTH_ORIF_EXPR:
      case TRUTH_AND_EXPR:
      case TRUTH_OR_EXPR:

Index: gcc/gcc/doc/invoke.texi
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/doc/invoke.texi,v
retrieving revision 1.543
diff -c -3 -p -r1.543 invoke.texi
*** gcc/gcc/doc/invoke.texi	5 Oct 2004 07:15:03 -0000	1.543
--- gcc/gcc/doc/invoke.texi	8 Oct 2004 14:31:27 -0000
*************** but @samp{x[(void)i,j]} will not.
*** 2653,2658 ****
--- 2653,2662 ----
  An unsigned value is compared against zero with @samp{<} or @samp{>=}.
  
  @item
+ An expression like @samp{a&&b} appears in a context where @samp{a&b}
+ may have been intended.
+ 
+ @item
  Storage-class specifiers like @code{static} are not the first things in
  a declaration.  According to the C Standard, this usage is obsolescent.
  


Index: gcc/gcc/testsuite/gcc.dg/wandand.c
===================================================================
diff -c3p /dev/null gcc/gcc/testsuite/gcc.dg/wandand.c
*** /dev/null	Fri Aug 30 19:31:37 2002
--- gcc/gcc/testsuite/gcc.dg/wandand.c	Fri Oct  8 09:36:45 2004
***************
*** 0 ****
--- 1,15 ----
+ /* { dg-do compile } */
+ /* { dg-options "-Wextra" } */
+ 
+ 
+ void subr (int a, char *p)
+ {
+   if (a && 0x200) return; /* { dg-warning "non-boolean constant" } */
+ 
+   if (p && 0x200) return; /* { dg-bogus "non-boolean constant" } */
+   if (a && p) return;     /* { dg-bogus "non-boolean constant" } */
+   if (a && 0) return;     /* { dg-bogus "non-boolean constant" } */
+   if (a && 1) return;     /* { dg-bogus "non-boolean constant" } */
+   if (3 && 4) return;     /* { dg-bogus "non-boolean constant" } */
+   if (a && 1.1) return;   /* { dg-bogus "non-boolean constant" } */
+ }