Skip to content

Commit dfdfd30

Browse files
xguptaxgupta
committed
[Clang] Fix -Wconstant-logical-operand when LHS is a constant
This fix PR37919 The below code produces -Wconstant-logical-operand for the first statement, but not the second. void foo(int x) { if (x && 5) {} if (5 && x) {} } Reviewed By: nickdesaulniers Differential Revision: https://reviews.llvm.org/D142609
1 parent f5768ec commit dfdfd30

File tree

9 files changed

+114
-43
lines changed

9 files changed

+114
-43
lines changed

‎clang/include/clang/Sema/Sema.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12679,6 +12679,8 @@ class Sema final {
1267912679
QualType CheckBitwiseOperands( // C99 6.5.[10...12]
1268012680
ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
1268112681
BinaryOperatorKind Opc);
12682+
void diagnoseLogicalInsteadOfBitwise(Expr *Op1, Expr *Op2, SourceLocation Loc,
12683+
BinaryOperatorKind Opc);
1268212684
QualType CheckLogicalOperands( // C99 6.5.[13,14]
1268312685
ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
1268412686
BinaryOperatorKind Opc);

‎clang/lib/Sema/SemaExpr.cpp

Lines changed: 58 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -13893,6 +13893,56 @@ inline QualType Sema::CheckBitwiseOperands(ExprResult &LHS, ExprResult &RHS,
1389313893
return InvalidOperands(Loc, LHS, RHS);
1389413894
}
1389513895

13896+
// Diagnose cases where the user write a logical and/or but probably meant a
13897+
// bitwise one. We do this when one of the operands is a non-bool integer and
13898+
// the other is a constant.
13899+
void Sema::diagnoseLogicalInsteadOfBitwise(Expr *Op1, Expr *Op2,
13900+
SourceLocation Loc,
13901+
BinaryOperatorKind Opc) {
13902+
if (Op1->getType()->isIntegerType() && !Op1->getType()->isBooleanType() &&
13903+
Op2->getType()->isIntegerType() && !Op2->isValueDependent() &&
13904+
// Don't warn in macros or template instantiations.
13905+
!Loc.isMacroID() && !inTemplateInstantiation() &&
13906+
!Op2->getExprLoc().isMacroID() &&
13907+
!Op1->getExprLoc().isMacroID()) {
13908+
bool IsOp1InMacro = Op1->getExprLoc().isMacroID();
13909+
bool IsOp2InMacro = Op2->getExprLoc().isMacroID();
13910+
13911+
// Exclude the specific expression from triggering the warning.
13912+
if (!(IsOp1InMacro && IsOp2InMacro && Op1->getSourceRange() == Op2->getSourceRange())) {
13913+
// If the RHS can be constant folded, and if it constant folds to something
13914+
// that isn't 0 or 1 (which indicate a potential logical operation that
13915+
// happened to fold to true/false) then warn.
13916+
// Parens on the RHS are ignored.
13917+
// If the RHS can be constant folded, and if it constant folds to something
13918+
// that isn't 0 or 1 (which indicate a potential logical operation that
13919+
// happened to fold to true/false) then warn.
13920+
// Parens on the RHS are ignored.
13921+
Expr::EvalResult EVResult;
13922+
if (Op2->EvaluateAsInt(EVResult, Context)) {
13923+
llvm::APSInt Result = EVResult.Val.getInt();
13924+
if ((getLangOpts().Bool && !Op2->getType()->isBooleanType() &&
13925+
!Op2->getExprLoc().isMacroID()) ||
13926+
(Result != 0 && Result != 1)) {
13927+
Diag(Loc, diag::warn_logical_instead_of_bitwise)
13928+
<< Op2->getSourceRange() << (Opc == BO_LAnd ? "&&" : "||");
13929+
// Suggest replacing the logical operator with the bitwise version
13930+
Diag(Loc, diag::note_logical_instead_of_bitwise_change_operator)
13931+
<< (Opc == BO_LAnd ? "&" : "|")
13932+
<< FixItHint::CreateReplacement(
13933+
SourceRange(Loc, getLocForEndOfToken(Loc)),
13934+
Opc == BO_LAnd ? "&" : "|");
13935+
if (Opc == BO_LAnd)
13936+
// Suggest replacing "Foo() && kNonZero" with "Foo()"
13937+
Diag(Loc, diag::note_logical_instead_of_bitwise_remove_constant)
13938+
<< FixItHint::CreateRemoval(SourceRange(
13939+
getLocForEndOfToken(Op1->getEndLoc()), Op2->getEndLoc()));
13940+
}
13941+
}
13942+
}
13943+
}
13944+
}
13945+
1389613946
// C99 6.5.[13,14]
1389713947
inline QualType Sema::CheckLogicalOperands(ExprResult &LHS, ExprResult &RHS,
1389813948
SourceLocation Loc,
@@ -13911,9 +13961,6 @@ inline QualType Sema::CheckLogicalOperands(ExprResult &LHS, ExprResult &RHS,
1391113961
}
1391213962
}
1391313963

13914-
if (EnumConstantInBoolContext)
13915-
Diag(Loc, diag::warn_enum_constant_in_bool_context);
13916-
1391713964
// WebAssembly tables can't be used with logical operators.
1391813965
QualType LHSTy = LHS.get()->getType();
1391913966
QualType RHSTy = RHS.get()->getType();
@@ -13924,40 +13971,14 @@ inline QualType Sema::CheckLogicalOperands(ExprResult &LHS, ExprResult &RHS,
1392413971
return InvalidOperands(Loc, LHS, RHS);
1392513972
}
1392613973

13927-
// Diagnose cases where the user write a logical and/or but probably meant a
13928-
// bitwise one. We do this when the LHS is a non-bool integer and the RHS
13929-
// is a constant.
13930-
if (!EnumConstantInBoolContext && LHS.get()->getType()->isIntegerType() &&
13931-
!LHS.get()->getType()->isBooleanType() &&
13932-
RHS.get()->getType()->isIntegerType() && !RHS.get()->isValueDependent() &&
13933-
// Don't warn in macros or template instantiations.
13934-
!Loc.isMacroID() && !inTemplateInstantiation()) {
13935-
// If the RHS can be constant folded, and if it constant folds to something
13936-
// that isn't 0 or 1 (which indicate a potential logical operation that
13937-
// happened to fold to true/false) then warn.
13938-
// Parens on the RHS are ignored.
13939-
Expr::EvalResult EVResult;
13940-
if (RHS.get()->EvaluateAsInt(EVResult, Context)) {
13941-
llvm::APSInt Result = EVResult.Val.getInt();
13942-
if ((getLangOpts().Bool && !RHS.get()->getType()->isBooleanType() &&
13943-
!RHS.get()->getExprLoc().isMacroID()) ||
13944-
(Result != 0 && Result != 1)) {
13945-
Diag(Loc, diag::warn_logical_instead_of_bitwise)
13946-
<< RHS.get()->getSourceRange() << (Opc == BO_LAnd ? "&&" : "||");
13947-
// Suggest replacing the logical operator with the bitwise version
13948-
Diag(Loc, diag::note_logical_instead_of_bitwise_change_operator)
13949-
<< (Opc == BO_LAnd ? "&" : "|")
13950-
<< FixItHint::CreateReplacement(
13951-
SourceRange(Loc, getLocForEndOfToken(Loc)),
13952-
Opc == BO_LAnd ? "&" : "|");
13953-
if (Opc == BO_LAnd)
13954-
// Suggest replacing "Foo() && kNonZero" with "Foo()"
13955-
Diag(Loc, diag::note_logical_instead_of_bitwise_remove_constant)
13956-
<< FixItHint::CreateRemoval(
13957-
SourceRange(getLocForEndOfToken(LHS.get()->getEndLoc()),
13958-
RHS.get()->getEndLoc()));
13959-
}
13960-
}
13974+
if (EnumConstantInBoolContext) {
13975+
// Warn when converting the enum constant to a boolean
13976+
Diag(Loc, diag::warn_enum_constant_in_bool_context);
13977+
} else {
13978+
// Diagnose cases where the user write a logical and/or but probably meant a
13979+
// bitwise one.
13980+
diagnoseLogicalInsteadOfBitwise(LHS.get(), RHS.get(), Loc, Opc);
13981+
diagnoseLogicalInsteadOfBitwise(RHS.get(), LHS.get(), Loc, Opc);
1396113982
}
1396213983

1396313984
if (!Context.getLangOpts().CPlusPlus) {

‎clang/test/C/drs/dr4xx.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,9 @@ void dr489(void) {
308308
case (int){ 2 }: break; /* expected-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}}
309309
c89only-warning {{compound literals are a C99-specific feature}}
310310
*/
311-
case 12 || main(): break; /* expected-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}} */
311+
case 12 || main(): break; /* expected-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}}
312+
expected-warning {{use of logical '||' with constant operand}} \
313+
expected-note {{use '|' for a bitwise operation}} */
312314
}
313315
}
314316

‎clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,15 @@ constexpr S InitList3(int a) { return a ? S{ a, a } : S{ a, ng }; }; // ok
7272

7373
constexpr int LogicalAnd1(int n) { return n && (throw, 0); } // ok
7474
constexpr int LogicalAnd2(int n) { return 1 && (throw, 0); } // expected-error {{never produces}} expected-note {{subexpression}}
75+
// expected-warning@-1 {{use of logical '&&' with constant operand}}
76+
// expected-note@-2 {{use '&' for a bitwise operation}}
77+
// expected-note@-3 {{remove constant to silence this warning}}
7578

7679
constexpr int LogicalOr1(int n) { return n || (throw, 0); } // ok
7780
constexpr int LogicalOr2(int n) { return 0 || (throw, 0); } // expected-error {{never produces}} expected-note {{subexpression}}
81+
// expected-warning@-1 {{use of logical '||' with constant operand}}
82+
// expected-note@-2 {{use '|' for a bitwise operation}}
83+
7884

7985
constexpr int Conditional1(bool b, int n) { return b ? n : ng; } // ok
8086
constexpr int Conditional2(bool b, int n) { return b ? n * ng : n + ng; } // expected-error {{never produces}} expected-note {{both arms of conditional operator are unable to produce a constant expression}}

‎clang/test/Parser/cxx2a-concept-declaration.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,11 @@ template<typename T> concept C16 = true && (0 && 0); // expected-error {{atomic
7979
// expected-warning@-1{{use of logical '&&' with constant operand}}
8080
// expected-note@-2{{use '&' for a bitwise operation}}
8181
// expected-note@-3{{remove constant to silence this warning}}
82+
// expected-warning@-4{{use of logical '&&' with constant operand}}
83+
// expected-note@-5{{use '&' for a bitwise operation}}
84+
// expected-note@-6{{remove constant to silence this warning}}
85+
86+
8287
template<typename T> concept C17 = T{};
8388
static_assert(!C17<bool>);
8489
template<typename T> concept C18 = (bool&&)true;

‎clang/test/Sema/exprs.c

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,10 @@ int test20(int x) {
212212
// expected-note {{use '&' for a bitwise operation}} \
213213
// expected-note {{remove constant to silence this warning}}
214214

215+
return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
216+
// expected-note {{use '&' for a bitwise operation}} \
217+
// expected-note {{remove constant to silence this warning}}
218+
215219
return x && sizeof(int) == 4; // no warning, RHS is logical op.
216220

217221
// no warning, this is an idiom for "true" in old C style.
@@ -223,6 +227,9 @@ int test20(int x) {
223227
// expected-note {{use '|' for a bitwise operation}}
224228
return x || 5; // expected-warning {{use of logical '||' with constant operand}} \
225229
// expected-note {{use '|' for a bitwise operation}}
230+
return 5 || x; // expected-warning {{use of logical '||' with constant operand}} \
231+
// expected-note {{use '|' for a bitwise operation}}
232+
226233
return x && 0;
227234
return x && 1;
228235
return x && -1; // expected-warning {{use of logical '&&' with constant operand}} \

‎clang/test/SemaCXX/expressions.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,9 @@ int test2(int x) {
4444
return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
4545
// expected-note {{use '&' for a bitwise operation}} \
4646
// expected-note {{remove constant to silence this warning}}
47+
return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
48+
// expected-note {{use '&' for a bitwise operation}} \
49+
// expected-note {{remove constant to silence this warning}}
4750

4851
return x && sizeof(int) == 4; // no warning, RHS is logical op.
4952
return x && true;
@@ -66,6 +69,8 @@ int test2(int x) {
6669
// expected-note {{use '|' for a bitwise operation}}
6770
return x || 5; // expected-warning {{use of logical '||' with constant operand}} \
6871
// expected-note {{use '|' for a bitwise operation}}
72+
return 5 || x; // expected-warning {{use of logical '||' with constant operand}} \
73+
// expected-note {{use '|' for a bitwise operation}}
6974
return x && 0; // expected-warning {{use of logical '&&' with constant operand}} \
7075
// expected-note {{use '&' for a bitwise operation}} \
7176
// expected-note {{remove constant to silence this warning}}
@@ -139,6 +144,5 @@ void test4() {
139144
bool r1 = X || Y;
140145

141146
#define Y2 2
142-
bool r2 = X || Y2; // expected-warning {{use of logical '||' with constant operand}} \
143-
// expected-note {{use '|' for a bitwise operation}}
147+
bool r2 = X || Y2;
144148
}

‎clang/test/SemaCXX/warn-unsequenced.cpp

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,15 +76,37 @@ void test() {
7676

7777
(xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
7878
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
79-
(0 && (a = 0)) + a; // ok
79+
80+
(0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
81+
// cxx11-note@-1 {{use '&' for a bitwise operation}}
82+
// cxx11-note@-2 {{remove constant to silence this warning}}
83+
// cxx17-warning@-3 {{use of logical '&&' with constant operand}}
84+
// cxx17-note@-4 {{use '&' for a bitwise operation}}
85+
// cxx17-note@-5 {{remove constant to silence this warning}}
86+
8087
(1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
8188
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
89+
// cxx11-warning@-2 {{use of logical '&&' with constant operand}}
90+
// cxx11-note@-3 {{use '&' for a bitwise operation}}
91+
// cxx11-note@-4 {{remove constant to silence this warning}}
92+
// cxx17-warning@-5 {{use of logical '&&' with constant operand}}
93+
// cxx17-note@-6 {{use '&' for a bitwise operation}}
94+
// cxx17-note@-7 {{remove constant to silence this warning}}
95+
8296

8397
(xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
8498
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
8599
(0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
86100
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
87-
(1 || (a = 0)) + a; // ok
101+
// cxx11-warning@-2 {{use of logical '||' with constant operand}}
102+
// cxx11-note@-3 {{use '|' for a bitwise operation}}
103+
// cxx17-warning@-4 {{use of logical '||' with constant operand}}
104+
// cxx17-note@-5 {{use '|' for a bitwise operation}}
105+
(1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
106+
// cxx11-note@-1 {{use '|' for a bitwise operation}}
107+
// cxx17-warning@-2 {{use of logical '||' with constant operand}}
108+
// cxx17-note@-3 {{use '|' for a bitwise operation}}
109+
88110

89111
(xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
90112
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}

‎clang/test/SemaTemplate/dependent-expr.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,9 @@ namespace PR7198 {
4343

4444
namespace PR7724 {
4545
template<typename OT> int myMethod()
46-
{ return 2 && sizeof(OT); }
46+
{ return 2 && sizeof(OT); } // expected-warning {{use of logical '&&' with constant operand}} \
47+
// expected-note {{use '&' for a bitwise operation}} \
48+
// expected-note {{remove constant to silence this warning}}
4749
}
4850

4951
namespace test4 {

0 commit comments

Comments
 (0)