Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[rtrieu]

Summary

Extend -Wparentheses to cover mixing bitwise-and and bitwise-or with the conditional operator. There's two main cases seen with this:

unsigned bits1 = 0xf0 | cond ? 0x4 : 0x1;
unsigned bits2 = cond1 ? 0xf0 : 0x10 | cond2 ? 0x5 : 0x2;

// Intended order of evaluation:
unsigned bits1 = 0xf0 | (cond ? 0x4 : 0x1);
unsigned bits2 = (cond1 ? 0xf0 : 0x10) | (cond2 ? 0x5 : 0x2);

// Actual order of evaluation:
unsigned bits1 = (0xf0 | cond) ? 0x4 : 0x1;
unsigned bits2 = cond1 ? 0xf0 : ((0x10 | cond2) ? 0x5 : 0x2);

[jfb]

test/Sema/parentheses.c

156

I don't understand why ^ is different. What do you mean by "often used as a logical xor`?

[MaskRay]

test/Sema/parentheses.c

148

I hope these | ? : & ? : warnings are disabled-by-default.

[rtrieu]

test/Sema/parentheses.c

148

These new warnings reuse the existing parentheses warnings, which is diag::warn_precedence_conditional. That is on by default, so this one as written is also on by default..

156

In C++, there's the bitwise operators |, &, and ^. And there's the logical operators || and &&. Since there's no ^^ for a logical-xor, many people will just use the bitwise-xor ^ instead. Since this isn't warning on logical operators, it makes sense to exclude the bitwise-xor that is often used as logical-xor.

[jfb]

test/Sema/parentheses.c

156

So code is often buggy when it uses | and & as diagnosed by this patch, but is rarely buggy when it uses ^?

[rtrieu]

test/Sema/parentheses.c

156

That's correct. From my testing, &&, || and ^ all had low bug finding rates and didn't make sense to include into this warning while | and & had a high bug finding rate.

[jfb]

test/Sema/parentheses.c

156

OK thanks for clarifying. Could you explain this instead of "logical xor"? It seems useful to know when reading your code that in your experience ^ wasn't a source of bugs as much as the others.

[rtrieu]

Update comments to explain why bitwise-xor is treated as a logical operator.

[rtrieu]

test/Sema/parentheses.c

156

I have added comments for this test and for the code where xor is excluded.

[jfb]

Thanks!

[MaskRay]

test/Sema/parentheses.c

148

I agree that

cond1 ? 0xf0 : 0x10 | cond2 ? 0x5 : 0x2; is confusing and justifies a warning. But what is tested here is different.

That is why I created D65192, because such warnings are very annoying as enabled-by-default diagnostics.

I think this change will make it even harder to remove some annoying -Wparentheses warnings..

[rtrieu]

test/Sema/parentheses.c

148

I can add tests for the other case. This one was used because it was shorter and easier to copy.

Would creating a parentheses subgroup (like -Wbitwise-conditional-parentheses), duping the warning into it, and marking it DefaultIgnore be a better alternative for you?

[rtrieu]

Add more test cases and split new warnings into a separate warning group, but still under -Wparentheses

[xbolva00]

Do you plan to land it?

lib/Sema/SemaExpr.cpp

if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID() &&
     // We can't display the parentheses, so just show the bare note.
     Self.Diag(Loc, Note) << ParenRange;
   }
 }
 
 static bool IsArithmeticOp(BinaryOperatorKind Opc) {
   return BinaryOperator::isAdditiveOp(Opc) ||
          BinaryOperator::isMultiplicativeOp(Opc) ||
-         BinaryOperator::isShiftOp(Opc);
+         BinaryOperator::isShiftOp(Opc) || Opc == BO_And || Opc == BO_Or;
+  // This only checks for bitwise-or and bitwise-and, but not bitwise-xor and
+  // not any of the logical operators.  Bitwise-xor is commonly used as a
+  // logical-xor because there is no logical-xor operator.  The logical
+  // operators, including uses of xor, have a high false positive rate for
+  // precedence warnings.
 }
 
 /// IsArithmeticBinaryExpr - Returns true if E is an arithmetic binary
 /// expression, either using a built-in or overloaded operator,
 /// and sets *OpCode to the opcode and *RHSExprs to the right-hand side
 /// expression.
 static bool IsArithmeticBinaryExpr(Expr *E, BinaryOperatorKind *Opcode,
                                    Expr **RHSExprs) {

test/Sema/parentheses.c

void conditional_op(int x, int y, _Bool b, void* p) {
 
   (void)(x + p ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
   (void)(p + x ? 1 : 2); // no warning
 
   (void)(p + b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
 
   (void)(x + y > 0 ? 1 : 2); // no warning
   (void)(x + (y > 0) ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '&'}} expected-note 2{{place parentheses}}
+
+  (void)((x | b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x | (b ? 1 : 2));  // no warning, has parentheses
+  (void)((x & b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x & (b ? 1 : 2));  // no warning, has parentheses
+
+  // Only warn on uses of the bitwise operators, and not the logical operators.
+  // The bitwise operators are more likely to be bugs while the logical
+  // operators are more likely to be used correctly.  Since there is no
+  // explicit logical-xor operator, the bitwise-xor is commonly used instead.
+  // For this warning, treat the bitwise-xor as if it were a logical operator.
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator
+  (void)(x && b ? 1 : 2);  // no warning, logical operator
 }
 
 // RUN: not %clang_cc1 -fsyntax-only -Wparentheses -Werror -fdiagnostics-show-option %s 2>&1 | FileCheck %s -check-prefix=CHECK-FLAG
 // CHECK-FLAG: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses]