[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
This revision was automatically updated to reflect the committed changes.
Closed by commit rG637af4cc3780: Add -Wbitwise-conditional-parentheses to warn
on mixing | and with ?: (authored by
rtrieu).
Herald added a project: clang.
Changed prior to commit:
https://reviews.llvm.org/D66043?vs=219213=225732#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66043/new/
https://reviews.llvm.org/D66043
Files:
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/parentheses.c
Index: clang/test/Sema/parentheses.c
===
--- clang/test/Sema/parentheses.c
+++ clang/test/Sema/parentheses.c
@@ -93,6 +93,28 @@
(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
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -7620,7 +7620,12 @@
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 @@
// 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);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5745,6 +5745,9 @@
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">;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -296,6 +296,7 @@
def FlexibleArrayExtensions : DiagGroup<"flexible-array-extensions">;
def FourByteMultiChar : DiagGroup<"four-char-constants">;
def GlobalConstructors :
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
xbolva00 added a comment. Do you plan to land it? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66043/new/ https://reviews.llvm.org/D66043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
rtrieu updated this revision to Diff 219213.
rtrieu added a comment.
Add more test cases and split new warnings into a separate warning group, but
still under -Wparentheses
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66043/new/
https://reviews.llvm.org/D66043
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -144,6 +144,28 @@
(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
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7485,7 +7485,12 @@
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
@@ -7575,7 +7580,11 @@
// 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);
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5618,6 +5618,9 @@
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">;
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -280,6 +280,7 @@
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:
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
rtrieu marked an inline comment as done.
rtrieu added inline comments.
Comment at: test/Sema/parentheses.c:148
+
+ (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}}
MaskRay wrote:
> rtrieu wrote:
> > MaskRay wrote:
> > > I hope these `| ? :` `& ? :` warnings are disabled-by-default.
> > 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..
> 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..
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?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66043/new/
https://reviews.llvm.org/D66043
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
MaskRay added inline comments.
Comment at: test/Sema/parentheses.c:148
+
+ (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}}
rtrieu wrote:
> MaskRay wrote:
> > I hope these `| ? :` `& ? :` warnings are disabled-by-default.
> 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..
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..
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66043/new/
https://reviews.llvm.org/D66043
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Herald added a subscriber: dexonsmith. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66043/new/ https://reviews.llvm.org/D66043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
rtrieu marked an inline comment as done. rtrieu added inline comments. Comment at: test/Sema/parentheses.c:156 + + (void)(x ^ b ? 1 : 2); // no warning, ^ is often used as logical xor + (void)(x || b ? 1 : 2); // no warning, logical operator jfb wrote: > rtrieu wrote: > > jfb wrote: > > > rtrieu wrote: > > > > jfb wrote: > > > > > I don't understand why `^` is different. What do you mean by "often > > > > > used as a logical xor`? > > > > 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. > > > So code is often buggy when it uses `|` and `&` as diagnosed by this > > > patch, but is rarely buggy when it uses `^`? > > 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. > 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. I have added comments for this test and for the code where xor is excluded. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66043/new/ https://reviews.llvm.org/D66043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
rtrieu updated this revision to Diff 214753.
rtrieu added a comment.
Update comments to explain why bitwise-xor is treated as a logical operator.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66043/new/
https://reviews.llvm.org/D66043
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -144,6 +144,23 @@
(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
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7485,7 +7485,12 @@
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
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -144,6 +144,23 @@
(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
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7485,7 +7485,12 @@
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
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
jfb added inline comments. Comment at: test/Sema/parentheses.c:156 + + (void)(x ^ b ? 1 : 2); // no warning, ^ is often used as logical xor + (void)(x || b ? 1 : 2); // no warning, logical operator rtrieu wrote: > jfb wrote: > > rtrieu wrote: > > > jfb wrote: > > > > I don't understand why `^` is different. What do you mean by "often > > > > used as a logical xor`? > > > 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. > > So code is often buggy when it uses `|` and `&` as diagnosed by this patch, > > but is rarely buggy when it uses `^`? > 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. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66043/new/ https://reviews.llvm.org/D66043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
rtrieu marked an inline comment as done. rtrieu added inline comments. Comment at: test/Sema/parentheses.c:156 + + (void)(x ^ b ? 1 : 2); // no warning, ^ is often used as logical xor + (void)(x || b ? 1 : 2); // no warning, logical operator jfb wrote: > rtrieu wrote: > > jfb wrote: > > > I don't understand why `^` is different. What do you mean by "often used > > > as a logical xor`? > > 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. > So code is often buggy when it uses `|` and `&` as diagnosed by this patch, > but is rarely buggy when it uses `^`? 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66043/new/ https://reviews.llvm.org/D66043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
jfb added inline comments. Comment at: test/Sema/parentheses.c:156 + + (void)(x ^ b ? 1 : 2); // no warning, ^ is often used as logical xor + (void)(x || b ? 1 : 2); // no warning, logical operator rtrieu wrote: > jfb wrote: > > I don't understand why `^` is different. What do you mean by "often used as > > a logical xor`? > 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. So code is often buggy when it uses `|` and `&` as diagnosed by this patch, but is rarely buggy when it uses `^`? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66043/new/ https://reviews.llvm.org/D66043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
rtrieu marked 2 inline comments as done.
rtrieu added inline comments.
Comment at: test/Sema/parentheses.c:148
+
+ (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}}
MaskRay wrote:
> I hope these `| ? :` `& ? :` warnings are disabled-by-default.
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..
Comment at: test/Sema/parentheses.c:156
+
+ (void)(x ^ b ? 1 : 2); // no warning, ^ is often used as logical xor
+ (void)(x || b ? 1 : 2); // no warning, logical operator
jfb wrote:
> I don't understand why `^` is different. What do you mean by "often used as a
> logical xor`?
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66043/new/
https://reviews.llvm.org/D66043
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
MaskRay added inline comments.
Comment at: test/Sema/parentheses.c:148
+
+ (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}}
I hope these `| ? :` `& ? :` warnings are disabled-by-default.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66043/new/
https://reviews.llvm.org/D66043
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
jfb added inline comments. Comment at: test/Sema/parentheses.c:156 + + (void)(x ^ b ? 1 : 2); // no warning, ^ is often used as logical xor + (void)(x || b ? 1 : 2); // no warning, logical operator I don't understand why `^` is different. What do you mean by "often used as a logical xor`? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66043/new/ https://reviews.llvm.org/D66043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
