From ae3e9c83d470f2f2c4fa84e66c0451abcbfb3598 Mon Sep 17 00:00:00 2001 From: Richard Trieu Date: Sat, 19 Oct 2019 01:47:49 +0000 Subject: Add -Wbitwise-conditional-parentheses to warn on mixing '|' and '&' with "?:" 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); Differential Revision: https://reviews.llvm.org/D66043 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@375326 91177308-0d34-0410-b5e6-96231b3b80d8 --- docs/ReleaseNotes.rst | 3 +++ include/clang/Basic/DiagnosticGroups.td | 2 ++ include/clang/Basic/DiagnosticSemaKinds.td | 3 +++ lib/Sema/SemaExpr.cpp | 13 +++++++++++-- test/Sema/parentheses.c | 22 ++++++++++++++++++++++ 5 files changed, 41 insertions(+), 2 deletions(-) diff --git a/docs/ReleaseNotes.rst b/docs/ReleaseNotes.rst index 41dea299e6..298d793a0d 100644 --- a/docs/ReleaseNotes.rst +++ b/docs/ReleaseNotes.rst @@ -61,6 +61,9 @@ Improvements to Clang's diagnostics 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 ------------------------------------------------- diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index 3573ebdfc6..9280595395 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -296,6 +296,7 @@ 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">; @@ -737,6 +738,7 @@ def ParenthesesOnEquality : DiagGroup<"parentheses-equality">; def Parentheses : DiagGroup<"parentheses", [LogicalOpParentheses, LogicalNotParentheses, + BitwiseConditionalParentheses, BitwiseOpParentheses, ShiftOpParentheses, OverloadedShiftOpParentheses, diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 98fce04e66..d802a92c42 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -5745,6 +5745,9 @@ def note_precedence_silence : Note< def warn_precedence_conditional : Warning< "operator '?:' has lower precedence than '%0'; '%0' will be evaluated first">, InGroup; +def warn_precedence_bitwise_conditional : Warning< + "operator '?:' has lower precedence than '%0'; '%0' will be evaluated first">, + InGroup; def note_precedence_conditional_first : Note< "place parentheses around the '?:' expression to evaluate it first">; diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 2734b4d076..e41cd5b665 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -7620,7 +7620,12 @@ static void SuggestParentheses(Sema &Self, SourceLocation Loc, 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 @@ -7710,7 +7715,11 @@ static void DiagnoseConditionalPrecedence(Sema &Self, // 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); diff --git a/test/Sema/parentheses.c b/test/Sema/parentheses.c index 0dabc11d39..047bcbfe6c 100644 --- a/test/Sema/parentheses.c +++ b/test/Sema/parentheses.c @@ -93,6 +93,28 @@ void conditional_op(int x, int y, _Bool b, void* p) { (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 -- cgit v1.2.1