Skip to content

Commit 78272d2

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 78272d2

File tree

2 files changed

+247
-20
lines changed

2 files changed

+247
-20
lines changed

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

Lines changed: 97 additions & 20 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,28 @@ static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const A
747747
return false;
748748
}
749749

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

787809
static bool isSameRawIdentifierToken(const Token &T1, const Token &T2,
788-
const SourceManager &SM) {
810+
const SourceManager &SM) {
789811
if (T1.getKind() != T2.getKind())
790812
return false;
791813
if (T1.isNot(tok::raw_identifier))
@@ -852,6 +874,58 @@ static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
852874

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

857931
void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -1089,7 +1163,6 @@ static bool exprEvaluatesToSymbolic(BinaryOperatorKind Opcode, APSInt Value) {
10891163
((Opcode == BO_And || Opcode == BO_AndAssign) && ~Value == 0);
10901164
}
10911165

1092-
10931166
void RedundantExpressionCheck::checkBitwiseExpr(
10941167
const MatchFinder::MatchResult &Result) {
10951168
if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>(
@@ -1134,8 +1207,8 @@ void RedundantExpressionCheck::checkBitwiseExpr(
11341207
ConstExpr))
11351208
return;
11361209

1137-
if((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID())
1138-
return;
1210+
if ((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID())
1211+
return;
11391212

11401213
SourceLocation Loc = IneffectiveOperator->getOperatorLoc();
11411214

@@ -1276,19 +1349,23 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
12761349
return;
12771350
}
12781351

1279-
if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
1352+
if (areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
1353+
BinOp, Result.Context)) {
12801354
const Expr *LhsConst = nullptr, *RhsConst = nullptr;
12811355
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;
1356+
if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
1357+
if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode,
1358+
LhsConst, RhsConst, Result.Context))
1359+
return;
1360+
1361+
if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
1362+
areExprsMacroAndNonMacro(LhsConst, RhsConst))
1363+
return;
1364+
} else {
1365+
if (!areExprsSameMacroOrLiteral(BinOp, Result.Context))
1366+
return;
1367+
}
12901368
}
1291-
12921369
diag(BinOp->getOperatorLoc(), "both sides of operator are equivalent");
12931370
}
12941371

‎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)