Skip to content

Commit

Permalink
Revert "[Clang] Fix -Wconstant-logical-operand when LHS is a constant"
Browse files Browse the repository at this point in the history 
This reverts commit dfdfd30.

An issue is reported for wrong warning, this has to be reconsidered.

Differential Revision: https://reviews.llvm.org/D157352
  • Loading branch information
@xgupta
xgupta committed Aug 8, 2023
1 parent 2f40e7b commit 862ec8a
Show file tree
Hide file tree
Showing 9 changed files with 43 additions and 114 deletions.
2 changes: 0 additions & 2 deletions clang/include/clang/Sema/Sema.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12694,8 +12694,6 @@ class Sema final {
QualType CheckBitwiseOperands( // C99 6.5.[10...12]
ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
BinaryOperatorKind Opc);
void diagnoseLogicalInsteadOfBitwise(Expr *Op1, Expr *Op2, SourceLocation Loc,
BinaryOperatorKind Opc);
QualType CheckLogicalOperands( // C99 6.5.[13,14]
ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
BinaryOperatorKind Opc);
Expand Down
95 changes: 37 additions & 58 deletions clang/lib/Sema/SemaExpr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13880,56 +13880,6 @@ inline QualType Sema::CheckBitwiseOperands(ExprResult &LHS, ExprResult &RHS,
return InvalidOperands(Loc, LHS, RHS);
}

// Diagnose cases where the user write a logical and/or but probably meant a
// bitwise one. We do this when one of the operands is a non-bool integer and
// the other is a constant.
void Sema::diagnoseLogicalInsteadOfBitwise(Expr *Op1, Expr *Op2,
SourceLocation Loc,
BinaryOperatorKind Opc) {
if (Op1->getType()->isIntegerType() && !Op1->getType()->isBooleanType() &&
Op2->getType()->isIntegerType() && !Op2->isValueDependent() &&
// Don't warn in macros or template instantiations.
!Loc.isMacroID() && !inTemplateInstantiation() &&
!Op2->getExprLoc().isMacroID() &&
!Op1->getExprLoc().isMacroID()) {
bool IsOp1InMacro = Op1->getExprLoc().isMacroID();
bool IsOp2InMacro = Op2->getExprLoc().isMacroID();

// Exclude the specific expression from triggering the warning.
if (!(IsOp1InMacro && IsOp2InMacro && Op1->getSourceRange() == Op2->getSourceRange())) {
// If the RHS can be constant folded, and if it constant folds to something
// that isn't 0 or 1 (which indicate a potential logical operation that
// happened to fold to true/false) then warn.
// Parens on the RHS are ignored.
// If the RHS can be constant folded, and if it constant folds to something
// that isn't 0 or 1 (which indicate a potential logical operation that
// happened to fold to true/false) then warn.
// Parens on the RHS are ignored.
Expr::EvalResult EVResult;
if (Op2->EvaluateAsInt(EVResult, Context)) {
llvm::APSInt Result = EVResult.Val.getInt();
if ((getLangOpts().Bool && !Op2->getType()->isBooleanType() &&
!Op2->getExprLoc().isMacroID()) ||
(Result != 0 && Result != 1)) {
Diag(Loc, diag::warn_logical_instead_of_bitwise)
<< Op2->getSourceRange() << (Opc == BO_LAnd ? "&&" : "||");
// Suggest replacing the logical operator with the bitwise version
Diag(Loc, diag::note_logical_instead_of_bitwise_change_operator)
<< (Opc == BO_LAnd ? "&" : "|")
<< FixItHint::CreateReplacement(
SourceRange(Loc, getLocForEndOfToken(Loc)),
Opc == BO_LAnd ? "&" : "|");
if (Opc == BO_LAnd)
// Suggest replacing "Foo() && kNonZero" with "Foo()"
Diag(Loc, diag::note_logical_instead_of_bitwise_remove_constant)
<< FixItHint::CreateRemoval(SourceRange(
getLocForEndOfToken(Op1->getEndLoc()), Op2->getEndLoc()));
}
}
}
}
}

// C99 6.5.[13,14]
inline QualType Sema::CheckLogicalOperands(ExprResult &LHS, ExprResult &RHS,
SourceLocation Loc,
Expand All @@ -13948,6 +13898,9 @@ inline QualType Sema::CheckLogicalOperands(ExprResult &LHS, ExprResult &RHS,
}
}

if (EnumConstantInBoolContext)
Diag(Loc, diag::warn_enum_constant_in_bool_context);

// WebAssembly tables can't be used with logical operators.
QualType LHSTy = LHS.get()->getType();
QualType RHSTy = RHS.get()->getType();
Expand All @@ -13958,14 +13911,40 @@ inline QualType Sema::CheckLogicalOperands(ExprResult &LHS, ExprResult &RHS,
return InvalidOperands(Loc, LHS, RHS);
}

if (EnumConstantInBoolContext) {
// Warn when converting the enum constant to a boolean
Diag(Loc, diag::warn_enum_constant_in_bool_context);
} else {
// Diagnose cases where the user write a logical and/or but probably meant a
// bitwise one.
diagnoseLogicalInsteadOfBitwise(LHS.get(), RHS.get(), Loc, Opc);
diagnoseLogicalInsteadOfBitwise(RHS.get(), LHS.get(), Loc, Opc);
// Diagnose cases where the user write a logical and/or but probably meant a
// bitwise one. We do this when the LHS is a non-bool integer and the RHS
// is a constant.
if (!EnumConstantInBoolContext && LHS.get()->getType()->isIntegerType() &&
!LHS.get()->getType()->isBooleanType() &&
RHS.get()->getType()->isIntegerType() && !RHS.get()->isValueDependent() &&
// Don't warn in macros or template instantiations.
!Loc.isMacroID() && !inTemplateInstantiation()) {
// If the RHS can be constant folded, and if it constant folds to something
// that isn't 0 or 1 (which indicate a potential logical operation that
// happened to fold to true/false) then warn.
// Parens on the RHS are ignored.
Expr::EvalResult EVResult;
if (RHS.get()->EvaluateAsInt(EVResult, Context)) {
llvm::APSInt Result = EVResult.Val.getInt();
if ((getLangOpts().Bool && !RHS.get()->getType()->isBooleanType() &&
!RHS.get()->getExprLoc().isMacroID()) ||
(Result != 0 && Result != 1)) {
Diag(Loc, diag::warn_logical_instead_of_bitwise)
<< RHS.get()->getSourceRange() << (Opc == BO_LAnd ? "&&" : "||");
// Suggest replacing the logical operator with the bitwise version
Diag(Loc, diag::note_logical_instead_of_bitwise_change_operator)
<< (Opc == BO_LAnd ? "&" : "|")
<< FixItHint::CreateReplacement(
SourceRange(Loc, getLocForEndOfToken(Loc)),
Opc == BO_LAnd ? "&" : "|");
if (Opc == BO_LAnd)
// Suggest replacing "Foo() && kNonZero" with "Foo()"
Diag(Loc, diag::note_logical_instead_of_bitwise_remove_constant)
<< FixItHint::CreateRemoval(
SourceRange(getLocForEndOfToken(LHS.get()->getEndLoc()),
RHS.get()->getEndLoc()));
}
}
}

if (!Context.getLangOpts().CPlusPlus) {
Expand Down
4 changes: 1 addition & 3 deletions clang/test/C/drs/dr4xx.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,9 +308,7 @@ void dr489(void) {
case (int){ 2 }: break; /* expected-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}}
c89only-warning {{compound literals are a C99-specific feature}}
*/
case 12 || main(): break; /* expected-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}}
expected-warning {{use of logical '||' with constant operand}} \
expected-note {{use '|' for a bitwise operation}} */
case 12 || main(): break; /* expected-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}} */
}
}

Expand Down
6 changes: 0 additions & 6 deletions clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,9 @@ constexpr S InitList3(int a) { return a ? S{ a, a } : S{ a, ng }; }; // ok

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

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


constexpr int Conditional1(bool b, int n) { return b ? n : ng; } // ok
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}}
Expand Down
5 changes: 0 additions & 5 deletions clang/test/Parser/cxx2a-concept-declaration.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,6 @@ template<typename T> concept C16 = true && (0 && 0); // expected-error {{atomic
// expected-warning@-1{{use of logical '&&' with constant operand}}
// expected-note@-2{{use '&' for a bitwise operation}}
// expected-note@-3{{remove constant to silence this warning}}
// expected-warning@-4{{use of logical '&&' with constant operand}}
// expected-note@-5{{use '&' for a bitwise operation}}
// expected-note@-6{{remove constant to silence this warning}}


template<typename T> concept C17 = T{};
static_assert(!C17<bool>);
template<typename T> concept C18 = (bool&&)true;
Expand Down
7 changes: 0 additions & 7 deletions clang/test/Sema/exprs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,6 @@ int test20(int x) {
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}

return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}

return x && sizeof(int) == 4; // no warning, RHS is logical op.

// no warning, this is an idiom for "true" in old C style.
Expand All @@ -227,9 +223,6 @@ int test20(int x) {
// expected-note {{use '|' for a bitwise operation}}
return x || 5; // expected-warning {{use of logical '||' with constant operand}} \
// expected-note {{use '|' for a bitwise operation}}
return 5 || x; // expected-warning {{use of logical '||' with constant operand}} \
// expected-note {{use '|' for a bitwise operation}}

return x && 0;
return x && 1;
return x && -1; // expected-warning {{use of logical '&&' with constant operand}} \
Expand Down
8 changes: 2 additions & 6 deletions clang/test/SemaCXX/expressions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,6 @@ int test2(int x) {
return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}
return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}

return x && sizeof(int) == 4; // no warning, RHS is logical op.
return x && true;
Expand All @@ -69,8 +66,6 @@ int test2(int x) {
// expected-note {{use '|' for a bitwise operation}}
return x || 5; // expected-warning {{use of logical '||' with constant operand}} \
// expected-note {{use '|' for a bitwise operation}}
return 5 || x; // expected-warning {{use of logical '||' with constant operand}} \
// expected-note {{use '|' for a bitwise operation}}
return x && 0; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}
Expand Down Expand Up @@ -144,5 +139,6 @@ void test4() {
bool r1 = X || Y;

#define Y2 2
bool r2 = X || Y2;
bool r2 = X || Y2; // expected-warning {{use of logical '||' with constant operand}} \
// expected-note {{use '|' for a bitwise operation}}
}
26 changes: 2 additions & 24 deletions clang/test/SemaCXX/warn-unsequenced.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,37 +76,15 @@ void test() {

(xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}

(0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
// cxx11-note@-1 {{use '&' for a bitwise operation}}
// cxx11-note@-2 {{remove constant to silence this warning}}
// cxx17-warning@-3 {{use of logical '&&' with constant operand}}
// cxx17-note@-4 {{use '&' for a bitwise operation}}
// cxx17-note@-5 {{remove constant to silence this warning}}

(0 && (a = 0)) + a; // ok
(1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
// cxx11-warning@-2 {{use of logical '&&' with constant operand}}
// cxx11-note@-3 {{use '&' for a bitwise operation}}
// cxx11-note@-4 {{remove constant to silence this warning}}
// cxx17-warning@-5 {{use of logical '&&' with constant operand}}
// cxx17-note@-6 {{use '&' for a bitwise operation}}
// cxx17-note@-7 {{remove constant to silence this warning}}


(xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
(0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
// cxx11-warning@-2 {{use of logical '||' with constant operand}}
// cxx11-note@-3 {{use '|' for a bitwise operation}}
// cxx17-warning@-4 {{use of logical '||' with constant operand}}
// cxx17-note@-5 {{use '|' for a bitwise operation}}
(1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
// cxx11-note@-1 {{use '|' for a bitwise operation}}
// cxx17-warning@-2 {{use of logical '||' with constant operand}}
// cxx17-note@-3 {{use '|' for a bitwise operation}}

(1 || (a = 0)) + a; // ok

(xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
Expand Down
4 changes: 1 addition & 3 deletions clang/test/SemaTemplate/dependent-expr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,7 @@ namespace PR7198 {

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

namespace test4 {
Expand Down

0 comments on commit 862ec8a

Please sign in to comment.