Skip to content

Commit abd7962

Browse files
Vladislav AranovEänolituri Lómitaurë
Vladislav Aranov
authored and
Eänolituri Lómitaurë
committed
[clang-tidy] Address false positives in misc-redundant-expression checker
Address situations when misc-redundant-expression checker provides excessive diagnostics for situations with different macros having the same value. In perticular situations described in the initial report of #118885 are addressed. The situations which are popped inside discussion like if (A + B == B + A) for macros are not properly addressed by this patch. But i still believe even in the current state the patch can be useful.
1 parent b7a6e9d commit abd7962

File tree

2 files changed

+249
-19
lines changed

2 files changed

+249
-19
lines changed

‎clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp‎

Lines changed: 99 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -139,10 +139,8 @@ static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
139139
return cast<BinaryOperator>(Left)->getOpcode() ==
140140
cast<BinaryOperator>(Right)->getOpcode();
141141
case Stmt::UnaryExprOrTypeTraitExprClass:
142-
const auto *LeftUnaryExpr =
143-
cast<UnaryExprOrTypeTraitExpr>(Left);
144-
const auto *RightUnaryExpr =
145-
cast<UnaryExprOrTypeTraitExpr>(Right);
142+
const auto *LeftUnaryExpr = cast<UnaryExprOrTypeTraitExpr>(Left);
143+
const auto *RightUnaryExpr = cast<UnaryExprOrTypeTraitExpr>(Right);
146144
if (LeftUnaryExpr->isArgumentType() && RightUnaryExpr->isArgumentType())
147145
return LeftUnaryExpr->getKind() == RightUnaryExpr->getKind() &&
148146
LeftUnaryExpr->getArgumentType() ==
@@ -699,7 +697,8 @@ static bool retrieveRelationalIntegerConstantExpr(
699697

700698
Symbol = OverloadedOperatorExpr->getArg(IntegerConstantIsFirstArg ? 1 : 0);
701699
OperandExpr = OverloadedOperatorExpr;
702-
Opcode = BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator());
700+
Opcode = BinaryOperator::getOverloadedOpcode(
701+
OverloadedOperatorExpr->getOperator());
703702

704703
if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr))
705704
return false;
@@ -728,7 +727,8 @@ static bool retrieveRelationalIntegerConstantExpr(
728727
}
729728

730729
// Checks for expressions like (X == 4) && (Y != 9)
731-
static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const ASTContext *AstCtx) {
730+
static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp,
731+
const ASTContext *AstCtx) {
732732
const auto *LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS());
733733
const auto *RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS());
734734

@@ -747,6 +747,31 @@ static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const A
747747
return false;
748748
}
749749

750+
static bool
751+
areSidesBinaryConstExpressionsOrDefines(const BinaryOperator *&BinOp,
752+
const ASTContext *AstCtx) {
753+
if (areSidesBinaryConstExpressions(BinOp, AstCtx))
754+
return true;
755+
756+
const auto *Lhs = BinOp->getLHS();
757+
const auto *Rhs = BinOp->getRHS();
758+
759+
if (!Lhs || !Rhs)
760+
return false;
761+
762+
auto IsDefineExpr = [AstCtx](const Expr *E) {
763+
SourceRange Lsr = E->getSourceRange();
764+
if (!Lsr.getBegin().isMacroID() || E->isValueDependent() ||
765+
!E->isIntegerConstantExpr(*AstCtx))
766+
return false;
767+
return true;
768+
};
769+
770+
if (IsDefineExpr(Lhs) || IsDefineExpr(Rhs))
771+
return true;
772+
return false;
773+
}
774+
750775
// Retrieves integer constant subexpressions from binary operator expressions
751776
// that have two equivalent sides.
752777
// E.g.: from (X == 5) && (X == 5) retrieves 5 and 5.
@@ -785,7 +810,7 @@ static bool retrieveConstExprFromBothSides(const BinaryOperator *&BinOp,
785810
}
786811

787812
static bool isSameRawIdentifierToken(const Token &T1, const Token &T2,
788-
const SourceManager &SM) {
813+
const SourceManager &SM) {
789814
if (T1.getKind() != T2.getKind())
790815
return false;
791816
if (T1.isNot(tok::raw_identifier))
@@ -852,6 +877,58 @@ static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
852877

853878
return LhsLoc.isMacroID() != RhsLoc.isMacroID();
854879
}
880+
881+
static bool areExprsSameMacroOrLiteral(const BinaryOperator *BinOp,
882+
const ASTContext *Context) {
883+
884+
if (!BinOp)
885+
return false;
886+
887+
const auto *Lhs = BinOp->getLHS();
888+
const auto *Rhs = BinOp->getRHS();
889+
const SourceManager &SM = Context->getSourceManager();
890+
891+
SourceRange Lsr = Lhs->getSourceRange();
892+
SourceRange Rsr = Rhs->getSourceRange();
893+
if (Lsr.getBegin().isMacroID()) {
894+
// Left is macro so right macro too
895+
if (Rsr.getBegin().isMacroID()) {
896+
// Both sides are macros so they are same macro or literal
897+
llvm::StringRef L = Lexer::getSourceText(
898+
CharSourceRange::getTokenRange(Lsr), SM, LangOptions(), 0);
899+
llvm::StringRef R = Lexer::getSourceText(
900+
CharSourceRange::getTokenRange(Rsr), SM, LangOptions(), 0);
901+
return L.compare(R) == 0;
902+
}
903+
// Left is macro but right is not so they are not same macro or literal
904+
return false;
905+
} else {
906+
const IntegerLiteral *Lil = dyn_cast<IntegerLiteral>(Lhs);
907+
const IntegerLiteral *Ril = dyn_cast<IntegerLiteral>(Rhs);
908+
if (Lil && Ril) {
909+
return Lil->getValue() == Ril->getValue();
910+
}
911+
912+
const StringLiteral *LStrl = dyn_cast<StringLiteral>(Lhs);
913+
const StringLiteral *RStrl = dyn_cast<StringLiteral>(Rhs);
914+
if (Lil && Ril) {
915+
llvm::StringRef L = Lexer::getSourceText(
916+
CharSourceRange::getTokenRange(LStrl->getBeginLoc()), SM,
917+
LangOptions(), 0);
918+
llvm::StringRef R = Lexer::getSourceText(
919+
CharSourceRange::getTokenRange(RStrl->getBeginLoc()), SM,
920+
LangOptions(), 0);
921+
return L.compare(R) == 0;
922+
}
923+
924+
const CXXBoolLiteralExpr *Lbl = dyn_cast<CXXBoolLiteralExpr>(Lhs);
925+
const CXXBoolLiteralExpr *Rbl = dyn_cast<CXXBoolLiteralExpr>(Rhs);
926+
if (Lbl && Rbl) {
927+
return Lbl->getValue() == Rbl->getValue();
928+
}
929+
}
930+
return false;
931+
}
855932
} // namespace
856933

857934
void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -1089,7 +1166,6 @@ static bool exprEvaluatesToSymbolic(BinaryOperatorKind Opcode, APSInt Value) {
10891166
((Opcode == BO_And || Opcode == BO_AndAssign) && ~Value == 0);
10901167
}
10911168

1092-
10931169
void RedundantExpressionCheck::checkBitwiseExpr(
10941170
const MatchFinder::MatchResult &Result) {
10951171
if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>(
@@ -1134,8 +1210,8 @@ void RedundantExpressionCheck::checkBitwiseExpr(
11341210
ConstExpr))
11351211
return;
11361212

1137-
if((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID())
1138-
return;
1213+
if ((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID())
1214+
return;
11391215

11401216
SourceLocation Loc = IneffectiveOperator->getOperatorLoc();
11411217

@@ -1276,17 +1352,21 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
12761352
return;
12771353
}
12781354

1279-
if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
1355+
if (areSidesBinaryConstExpressionsOrDefines(BinOp, Result.Context)) {
12801356
const Expr *LhsConst = nullptr, *RhsConst = nullptr;
12811357
BinaryOperatorKind MainOpcode{}, SideOpcode{};
1282-
1283-
if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode,
1284-
LhsConst, RhsConst, Result.Context))
1285-
return;
1286-
1287-
if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
1288-
areExprsMacroAndNonMacro(LhsConst, RhsConst))
1289-
return;
1358+
if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
1359+
if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode,
1360+
LhsConst, RhsConst, Result.Context))
1361+
return;
1362+
1363+
if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
1364+
areExprsMacroAndNonMacro(LhsConst, RhsConst))
1365+
return;
1366+
} else {
1367+
if (!areExprsSameMacroOrLiteral(BinOp, Result.Context))
1368+
return;
1369+
}
12901370
}
12911371

12921372
diag(BinOp->getOperatorLoc(), "both sides of operator are equivalent");

‎clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- -fno-delayed-template-parsing -Wno-array-compare-cxx26
2+
// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- -fno-delayed-template-parsing -Wno-array-compare-cxx26 -DTEST_MACRO
23

34
typedef __INT64_TYPE__ I64;
45

@@ -91,6 +92,155 @@ int TestSimpleEquivalent(int X, int Y) {
9192
return 0;
9293
}
9394

95+
#ifndef TEST_MACRO
96+
#define VAL_1 2
97+
#define VAL_3 3
98+
#else
99+
#define VAL_1 3
100+
#define VAL_3 2
101+
#endif
102+
103+
#define VAL_2 2
104+
105+
#ifndef TEST_MACRO
106+
#define VAL_4 2 + 1
107+
#define VAL_6 3 + 1
108+
#else
109+
#define VAL_4 3 + 1
110+
#define VAL_6 2 + 1
111+
#endif
112+
113+
#define VAL_5 2 + 1
114+
115+
struct TestStruct
116+
{
117+
int mA;
118+
int mB;
119+
int mC[10];
120+
};
121+
122+
int TestDefineEquivalent() {
123+
124+
int int_val1 = 3;
125+
int int_val2 = 4;
126+
int int_val = 0;
127+
const int cint_val2 = 4;
128+
129+
// Cases which should not be reported
130+
if (VAL_1 != VAL_2) return 0;
131+
if (VAL_3 != VAL_2) return 0;
132+
if (VAL_1 == VAL_2) return 0;
133+
if (VAL_3 == VAL_2) return 0;
134+
if (VAL_1 >= VAL_2) return 0;
135+
if (VAL_3 >= VAL_2) return 0;
136+
if (VAL_1 <= VAL_2) return 0;
137+
if (VAL_3 <= VAL_2) return 0;
138+
if (VAL_1 < VAL_2) return 0;
139+
if (VAL_3 < VAL_2) return 0;
140+
if (VAL_1 > VAL_2) return 0;
141+
if (VAL_3 > VAL_2) return 0;
142+
143+
if (VAL_4 != VAL_5) return 0;
144+
if (VAL_6 != VAL_5) return 0;
145+
if (VAL_6 == VAL_5) return 0;
146+
if (VAL_4 >= VAL_5) return 0;
147+
if (VAL_6 >= VAL_5) return 0;
148+
if (VAL_4 <= VAL_5) return 0;
149+
if (VAL_6 <= VAL_5) return 0;
150+
if (VAL_4 > VAL_5) return 0;
151+
if (VAL_6 > VAL_5) return 0;
152+
if (VAL_4 < VAL_5) return 0;
153+
if (VAL_6 < VAL_5) return 0;
154+
155+
if (VAL_1 != 2) return 0;
156+
if (VAL_3 == 3) return 0;
157+
158+
if (VAL_1 >= VAL_1) return 0;
159+
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
160+
if (VAL_2 <= VAL_2) return 0;
161+
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
162+
if (VAL_3 > VAL_3) return 0;
163+
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
164+
if (VAL_4 < VAL_4) return 0;
165+
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
166+
if (VAL_6 == VAL_6) return 2;
167+
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
168+
if (VAL_5 != VAL_5) return 2;
169+
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
170+
171+
if (1 >= 1) return 0;
172+
// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
173+
if (0xFF <= 0xFF) return 0;
174+
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: both sides of operator are equivalent
175+
if (042 > 042) return 0;
176+
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: both sides of operator are equivalent
177+
178+
int_val = (VAL_6 == VAL_6)?int_val1: int_val2;
179+
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both sides of operator are equivalent
180+
int_val = (042 > 042)?int_val1: int_val2;
181+
// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both sides of operator are equivalent
182+
183+
184+
// Ternary operator cases which should not be reported
185+
int_val = (VAL_4 == VAL_5)? int_val1: int_val2;
186+
int_val = (VAL_3 != VAL_2)? int_val1: int_val2;
187+
int_val = (VAL_6 != 10)? int_val1: int_val2;
188+
int_val = (VAL_6 != 3)? int_val1: int_val2;
189+
int_val = (VAL_6 != 4)? int_val1: int_val2;
190+
int_val = (VAL_6 == 3)? int_val1: int_val2;
191+
int_val = (VAL_6 == 4)? int_val1: int_val2;
192+
193+
TestStruct tsVar1 = {
194+
.mA = 3,
195+
.mB = int_val,
196+
.mC[0 ... VAL_2 - 2] = int_val + 1,
197+
};
198+
199+
TestStruct tsVar2 = {
200+
.mA = 3,
201+
.mB = int_val,
202+
.mC[0 ... cint_val2 - 2] = int_val + 1,
203+
};
204+
205+
TestStruct tsVar3 = {
206+
.mA = 3,
207+
.mB = int_val,
208+
.mC[0 ... VAL_3 - VAL_3] = int_val + 1,
209+
// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: both sides of operator are equivalent
210+
};
211+
212+
TestStruct tsVar4 = {
213+
.mA = 3,
214+
.mB = int_val,
215+
.mC[0 ... 5 - 5] = int_val + 1,
216+
// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: both sides of operator are equivalent
217+
};
218+
219+
return 1 + int_val + sizeof(tsVar1) + sizeof(tsVar2) +
220+
sizeof(tsVar3) + sizeof(tsVar4);
221+
}
222+
223+
#define LOOP_DEFINE 1
224+
225+
unsigned int testLoops(const unsigned int arr1[LOOP_DEFINE])
226+
{
227+
unsigned int localIndex;
228+
for (localIndex = LOOP_DEFINE - 1; localIndex > 0; localIndex--)
229+
{
230+
}
231+
for (localIndex = LOOP_DEFINE - 1; 10 > 10; localIndex--)
232+
// CHECK-MESSAGES: :[[@LINE-1]]:41: warning: both sides of operator are equivalent
233+
{
234+
}
235+
236+
for (localIndex = LOOP_DEFINE - 1; LOOP_DEFINE > LOOP_DEFINE; localIndex--)
237+
// CHECK-MESSAGES: :[[@LINE-1]]:50: warning: both sides of operator are equivalent
238+
{
239+
}
240+
241+
return localIndex;
242+
}
243+
94244
template <int DX>
95245
int TestSimpleEquivalentDependent() {
96246
if (DX > 0 && DX > 0) return 1;

0 commit comments

Comments
 (0)