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 ↗

(On Diff #214479)

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

[MaskRay]

test/Sema/parentheses.c

148 ↗

(On Diff #214479)

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

[rtrieu]

test/Sema/parentheses.c

148 ↗

(On Diff #214479)

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 ↗

(On Diff #214479)

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 ↗

(On Diff #214479)

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 ↗

(On Diff #214479)

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 ↗

(On Diff #214479)

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 ↗

(On Diff #214479)

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

[jfb]

Thanks!

[MaskRay]

test/Sema/parentheses.c

148 ↗

(On Diff #214479)

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 ↗

(On Diff #214479)

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?

clang/docs/ReleaseNotes.rst

 - -Wtautological-compare for self comparisons and
   -Wtautological-overlap-compare will now look through member and array
   access to determine if two operand expressions are the same.
 - -Wtautological-bitwise-compare is a new warning group.  This group has the
   current warning which diagnoses the tautological comparison of a bitwise
   operation and a constant.  The group also has the new warning which diagnoses
   when a bitwise-or with a non-negative value is converted to a bool, since
   that bool will always be true.
+- -Wbitwise-conditional-parentheses will warn on operator precedence issues
+  when mixing bitwise-and (&) and bitwise-or (|) operator with the
+  conditional operator (?:).
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 * In both C and C++ (C17 ``6.5.6p8``, C++ ``[expr.add]``), pointer arithmetic is
   only permitted within arrays. In particular, the behavior of a program is not
   defined if it adds a non-zero offset (or in C, any offset) to a null pointer,
   or if it forms a null pointer by subtracting an integer from a non-null

clang/include/clang/Basic/DiagnosticGroups.td

 def CXX2aCompat : DiagGroup<"c++2a-compat">;
 def CXX2aCompatPedantic : DiagGroup<"c++2a-compat-pedantic",
                                     [CXX2aCompat]>;
 
 def ExitTimeDestructors : DiagGroup<"exit-time-destructors">;
 def FlexibleArrayExtensions : DiagGroup<"flexible-array-extensions">;
 def FourByteMultiChar : DiagGroup<"four-char-constants">;
 def GlobalConstructors : DiagGroup<"global-constructors">;
+def BitwiseConditionalParentheses: DiagGroup<"bitwise-conditional-parentheses">;
 def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">;
 def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;
 def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">;
 def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;
 def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">;
 def DanglingElse: DiagGroup<"dangling-else">;
 def DanglingField : DiagGroup<"dangling-field">;
 def DanglingInitializerList : DiagGroup<"dangling-initializer-list">;

 // -Widiomatic-parentheses contains warnings about 'idiomatic'
 // missing parentheses;  it is off by default.  We do not include it
 // in -Wparentheses because most users who use -Wparentheses explicitly
 // do not want these warnings.
 def ParenthesesOnEquality : DiagGroup<"parentheses-equality">;
 def Parentheses : DiagGroup<"parentheses",
                             [LogicalOpParentheses,
                              LogicalNotParentheses,
+                             BitwiseConditionalParentheses,
                              BitwiseOpParentheses,
                              ShiftOpParentheses,
                              OverloadedShiftOpParentheses,
                              ParenthesesOnEquality,
                              DanglingElse]>;
 
 // -Wconversion has its own warnings, but we split a few out for
 // legacy reasons:

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def note_precedence_bitwise_first : Note<
   "place parentheses around the %0 expression to evaluate it first">;
 def note_precedence_silence : Note<
   "place parentheses around the '%0' expression to silence this warning">;
 
 def warn_precedence_conditional : Warning<
   "operator '?:' has lower precedence than '%0'; '%0' will be evaluated first">,
   InGroup<Parentheses>;
+def warn_precedence_bitwise_conditional : Warning<
+  "operator '?:' has lower precedence than '%0'; '%0' will be evaluated first">,
+  InGroup<BitwiseConditionalParentheses>;
 def note_precedence_conditional_first : Note<
   "place parentheses around the '?:' expression to evaluate it first">;
 
 def warn_enum_constant_in_bool_context : Warning<
   "converting the enum constant to a boolean">,
   InGroup<IntInBoolContext>, DefaultIgnore;
 def warn_left_shift_in_bool_context : Warning<
   "converting the result of '<<' to a boolean; did you mean '(%0) != 0'?">,

clang/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) {
static void DiagnoseConditionalPrecedence(Sema &Self,
   if (!IsArithmeticBinaryExpr(Condition, &CondOpcode, &CondRHS))
     return;
   if (!ExprLooksBoolean(CondRHS))
     return;
 
   // The condition is an arithmetic binary expression, with a right-
   // hand side that looks boolean, so warn.
 
-  Self.Diag(OpLoc, diag::warn_precedence_conditional)
+  unsigned DiagID = BinaryOperator::isBitwiseOp(CondOpcode)
+                        ? diag::warn_precedence_bitwise_conditional
+                        : diag::warn_precedence_conditional;
+
+  Self.Diag(OpLoc, DiagID)
       << Condition->getSourceRange()
       << BinaryOperator::getOpcodeStr(CondOpcode);
 
   SuggestParentheses(
       Self, OpLoc,
       Self.PDiag(diag::note_precedence_silence)
           << BinaryOperator::getOpcodeStr(CondOpcode),
       SourceRange(Condition->getBeginLoc(), Condition->getEndLoc()));

clang/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)(b ? 0xf0 : 0x10 | b ? 0x5 : 0x2); // expected-warning {{operator '?:' has lower precedence than '|'}} expected-note 2{{place parentheses}}
+
+  (void)((b ? 0xf0 : 0x10) | (b ? 0x5 : 0x2));  // no warning, has parentheses
+  (void)(b ? 0xf0 : (0x10 | b) ? 0x5 : 0x2);  // no warning, has 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]