Skip to content

Commit 8ac2b77

Browse files
vabridgersVince Bridgers
vabridgers
and
Vince Bridgers
authored
[analyzer] Remove alpha.core.IdenticalExpr Checker (#114715)
This change removes the alpha.core.IdenticalExpr static analysis checker since it's checks are present in the clang-tidy checks misc-redundant-expression and bugprone-branch-clone. This check was implemented as a static analysis check using AST matching, and since alpha and duplicated in 2 clang-tidy checks may be removed. Co-authored-by: Vince Bridgers <[email protected]>
1 parent 52690db commit 8ac2b77

File tree

11 files changed

+570
-663
lines changed

11 files changed

+570
-663
lines changed

‎clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp‎

Lines changed: 221 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,212 @@ void BranchCloneCheck::registerMatchers(MatchFinder *Finder) {
102102
this);
103103
Finder->addMatcher(switchStmt().bind("switch"), this);
104104
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
105+
Finder->addMatcher(
106+
ifStmt((hasThen(hasDescendant(ifStmt())))).bind("ifWithDescendantIf"),
107+
this);
108+
}
109+
110+
/// Determines whether two statement trees are identical regarding
111+
/// operators and symbols.
112+
///
113+
/// Exceptions: expressions containing macros or functions with possible side
114+
/// effects are never considered identical.
115+
/// Limitations: (t + u) and (u + t) are not considered identical.
116+
/// t*(u + t) and t*u + t*t are not considered identical.
117+
///
118+
static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
119+
const Stmt *Stmt2, bool IgnoreSideEffects) {
120+
121+
if (!Stmt1 || !Stmt2)
122+
return !Stmt1 && !Stmt2;
123+
124+
// If Stmt1 & Stmt2 are of different class then they are not
125+
// identical statements.
126+
if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
127+
return false;
128+
129+
const auto *Expr1 = dyn_cast<Expr>(Stmt1);
130+
const auto *Expr2 = dyn_cast<Expr>(Stmt2);
131+
132+
if (Expr1 && Expr2) {
133+
// If Stmt1 has side effects then don't warn even if expressions
134+
// are identical.
135+
if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx) &&
136+
Expr2->HasSideEffects(Ctx))
137+
return false;
138+
// If either expression comes from a macro then don't warn even if
139+
// the expressions are identical.
140+
if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
141+
return false;
142+
143+
// If all children of two expressions are identical, return true.
144+
Expr::const_child_iterator I1 = Expr1->child_begin();
145+
Expr::const_child_iterator I2 = Expr2->child_begin();
146+
while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
147+
if (!isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
148+
return false;
149+
++I1;
150+
++I2;
151+
}
152+
// If there are different number of children in the statements, return
153+
// false.
154+
if (I1 != Expr1->child_end())
155+
return false;
156+
if (I2 != Expr2->child_end())
157+
return false;
158+
}
159+
160+
switch (Stmt1->getStmtClass()) {
161+
default:
162+
return false;
163+
case Stmt::CallExprClass:
164+
case Stmt::ArraySubscriptExprClass:
165+
case Stmt::ArraySectionExprClass:
166+
case Stmt::OMPArrayShapingExprClass:
167+
case Stmt::OMPIteratorExprClass:
168+
case Stmt::ImplicitCastExprClass:
169+
case Stmt::ParenExprClass:
170+
case Stmt::BreakStmtClass:
171+
case Stmt::ContinueStmtClass:
172+
case Stmt::NullStmtClass:
173+
return true;
174+
case Stmt::CStyleCastExprClass: {
175+
const auto *CastExpr1 = cast<CStyleCastExpr>(Stmt1);
176+
const auto *CastExpr2 = cast<CStyleCastExpr>(Stmt2);
177+
178+
return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
179+
}
180+
case Stmt::ReturnStmtClass: {
181+
const auto *ReturnStmt1 = cast<ReturnStmt>(Stmt1);
182+
const auto *ReturnStmt2 = cast<ReturnStmt>(Stmt2);
183+
184+
return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
185+
ReturnStmt2->getRetValue(), IgnoreSideEffects);
186+
}
187+
case Stmt::ForStmtClass: {
188+
const auto *ForStmt1 = cast<ForStmt>(Stmt1);
189+
const auto *ForStmt2 = cast<ForStmt>(Stmt2);
190+
191+
if (!isIdenticalStmt(Ctx, ForStmt1->getInit(), ForStmt2->getInit(),
192+
IgnoreSideEffects))
193+
return false;
194+
if (!isIdenticalStmt(Ctx, ForStmt1->getCond(), ForStmt2->getCond(),
195+
IgnoreSideEffects))
196+
return false;
197+
if (!isIdenticalStmt(Ctx, ForStmt1->getInc(), ForStmt2->getInc(),
198+
IgnoreSideEffects))
199+
return false;
200+
if (!isIdenticalStmt(Ctx, ForStmt1->getBody(), ForStmt2->getBody(),
201+
IgnoreSideEffects))
202+
return false;
203+
return true;
204+
}
205+
case Stmt::DoStmtClass: {
206+
const auto *DStmt1 = cast<DoStmt>(Stmt1);
207+
const auto *DStmt2 = cast<DoStmt>(Stmt2);
208+
209+
if (!isIdenticalStmt(Ctx, DStmt1->getCond(), DStmt2->getCond(),
210+
IgnoreSideEffects))
211+
return false;
212+
if (!isIdenticalStmt(Ctx, DStmt1->getBody(), DStmt2->getBody(),
213+
IgnoreSideEffects))
214+
return false;
215+
return true;
216+
}
217+
case Stmt::WhileStmtClass: {
218+
const auto *WStmt1 = cast<WhileStmt>(Stmt1);
219+
const auto *WStmt2 = cast<WhileStmt>(Stmt2);
220+
221+
if (!isIdenticalStmt(Ctx, WStmt1->getCond(), WStmt2->getCond(),
222+
IgnoreSideEffects))
223+
return false;
224+
if (!isIdenticalStmt(Ctx, WStmt1->getBody(), WStmt2->getBody(),
225+
IgnoreSideEffects))
226+
return false;
227+
return true;
228+
}
229+
case Stmt::IfStmtClass: {
230+
const auto *IStmt1 = cast<IfStmt>(Stmt1);
231+
const auto *IStmt2 = cast<IfStmt>(Stmt2);
232+
233+
if (!isIdenticalStmt(Ctx, IStmt1->getCond(), IStmt2->getCond(),
234+
IgnoreSideEffects))
235+
return false;
236+
if (!isIdenticalStmt(Ctx, IStmt1->getThen(), IStmt2->getThen(),
237+
IgnoreSideEffects))
238+
return false;
239+
if (!isIdenticalStmt(Ctx, IStmt1->getElse(), IStmt2->getElse(),
240+
IgnoreSideEffects))
241+
return false;
242+
return true;
243+
}
244+
case Stmt::CompoundStmtClass: {
245+
const auto *CompStmt1 = cast<CompoundStmt>(Stmt1);
246+
const auto *CompStmt2 = cast<CompoundStmt>(Stmt2);
247+
248+
if (CompStmt1->size() != CompStmt2->size())
249+
return false;
250+
251+
if (!llvm::all_of(llvm::zip(CompStmt1->body(), CompStmt2->body()),
252+
[&Ctx, IgnoreSideEffects](
253+
std::tuple<const Stmt *, const Stmt *> stmtPair) {
254+
const Stmt *stmt0 = std::get<0>(stmtPair);
255+
const Stmt *stmt1 = std::get<1>(stmtPair);
256+
return isIdenticalStmt(Ctx, stmt0, stmt1,
257+
IgnoreSideEffects);
258+
})) {
259+
return false;
260+
}
261+
262+
return true;
263+
}
264+
case Stmt::CompoundAssignOperatorClass:
265+
case Stmt::BinaryOperatorClass: {
266+
const auto *BinOp1 = cast<BinaryOperator>(Stmt1);
267+
const auto *BinOp2 = cast<BinaryOperator>(Stmt2);
268+
return BinOp1->getOpcode() == BinOp2->getOpcode();
269+
}
270+
case Stmt::CharacterLiteralClass: {
271+
const auto *CharLit1 = cast<CharacterLiteral>(Stmt1);
272+
const auto *CharLit2 = cast<CharacterLiteral>(Stmt2);
273+
return CharLit1->getValue() == CharLit2->getValue();
274+
}
275+
case Stmt::DeclRefExprClass: {
276+
const auto *DeclRef1 = cast<DeclRefExpr>(Stmt1);
277+
const auto *DeclRef2 = cast<DeclRefExpr>(Stmt2);
278+
return DeclRef1->getDecl() == DeclRef2->getDecl();
279+
}
280+
case Stmt::IntegerLiteralClass: {
281+
const auto *IntLit1 = cast<IntegerLiteral>(Stmt1);
282+
const auto *IntLit2 = cast<IntegerLiteral>(Stmt2);
283+
284+
llvm::APInt I1 = IntLit1->getValue();
285+
llvm::APInt I2 = IntLit2->getValue();
286+
if (I1.getBitWidth() != I2.getBitWidth())
287+
return false;
288+
return I1 == I2;
289+
}
290+
case Stmt::FloatingLiteralClass: {
291+
const auto *FloatLit1 = cast<FloatingLiteral>(Stmt1);
292+
const auto *FloatLit2 = cast<FloatingLiteral>(Stmt2);
293+
return FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue());
294+
}
295+
case Stmt::StringLiteralClass: {
296+
const auto *StringLit1 = cast<StringLiteral>(Stmt1);
297+
const auto *StringLit2 = cast<StringLiteral>(Stmt2);
298+
return StringLit1->getBytes() == StringLit2->getBytes();
299+
}
300+
case Stmt::MemberExprClass: {
301+
const auto *MemberStmt1 = cast<MemberExpr>(Stmt1);
302+
const auto *MemberStmt2 = cast<MemberExpr>(Stmt2);
303+
return MemberStmt1->getMemberDecl() == MemberStmt2->getMemberDecl();
304+
}
305+
case Stmt::UnaryOperatorClass: {
306+
const auto *UnaryOp1 = cast<UnaryOperator>(Stmt1);
307+
const auto *UnaryOp2 = cast<UnaryOperator>(Stmt2);
308+
return UnaryOp1->getOpcode() == UnaryOp2->getOpcode();
309+
}
310+
}
105311
}
106312

107313
void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
@@ -269,6 +475,21 @@ void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
269475
return;
270476
}
271477

478+
if (const auto *IS = Result.Nodes.getNodeAs<IfStmt>("ifWithDescendantIf")) {
479+
const Stmt *Then = IS->getThen();
480+
auto CS = dyn_cast<CompoundStmt>(Then);
481+
if (CS && (!CS->body_empty())) {
482+
const auto *InnerIf = dyn_cast<IfStmt>(*CS->body_begin());
483+
if (InnerIf && isIdenticalStmt(Context, IS->getCond(), InnerIf->getCond(),
484+
/*IgnoreSideEffects=*/false)) {
485+
diag(IS->getBeginLoc(), "if with identical inner if statement");
486+
diag(InnerIf->getBeginLoc(), "inner if starts here",
487+
DiagnosticIDs::Note);
488+
}
489+
}
490+
return;
491+
}
492+
272493
llvm_unreachable("No if statement and no switch statement.");
273494
}
274495

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

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -855,9 +855,6 @@ static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
855855
} // namespace
856856

857857
void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
858-
const auto AnyLiteralExpr = ignoringParenImpCasts(
859-
anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral()));
860-
861858
const auto BannedIntegerLiteral =
862859
integerLiteral(expandedByMacro(KnownBannedMacroNames));
863860
const auto IsInUnevaluatedContext = expr(anyOf(
@@ -866,19 +863,16 @@ void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
866863
// Binary with equivalent operands, like (X != 2 && X != 2).
867864
Finder->addMatcher(
868865
traverse(TK_AsIs,
869-
binaryOperator(
870-
anyOf(isComparisonOperator(),
871-
hasAnyOperatorName("-", "/", "%", "|", "&", "^", "&&",
872-
"||", "=")),
873-
operandsAreEquivalent(),
874-
// Filter noisy false positives.
875-
unless(isInTemplateInstantiation()),
876-
unless(binaryOperatorIsInMacro()),
877-
unless(hasType(realFloatingPointType())),
878-
unless(hasEitherOperand(hasType(realFloatingPointType()))),
879-
unless(hasLHS(AnyLiteralExpr)),
880-
unless(hasDescendant(BannedIntegerLiteral)),
881-
unless(IsInUnevaluatedContext))
866+
binaryOperator(anyOf(isComparisonOperator(),
867+
hasAnyOperatorName("-", "/", "%", "|", "&",
868+
"^", "&&", "||", "=")),
869+
operandsAreEquivalent(),
870+
// Filter noisy false positives.
871+
unless(isInTemplateInstantiation()),
872+
unless(binaryOperatorIsInMacro()),
873+
unless(hasAncestor(arraySubscriptExpr())),
874+
unless(hasDescendant(BannedIntegerLiteral)),
875+
unless(IsInUnevaluatedContext))
882876
.bind("binary")),
883877
this);
884878

@@ -1238,6 +1232,50 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
12381232
if (const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binary")) {
12391233
// If the expression's constants are macros, check whether they are
12401234
// intentional.
1235+
1236+
//
1237+
// Special case for floating-point representation.
1238+
//
1239+
// If expressions on both sides of comparison operator are of type float,
1240+
// then for some comparison operators no warning shall be
1241+
// reported even if the expressions are identical from a symbolic point of
1242+
// view. Comparison between expressions, declared variables and literals
1243+
// are treated differently.
1244+
//
1245+
// != and == between float literals that have the same value should NOT
1246+
// warn. < > between float literals that have the same value SHOULD warn.
1247+
//
1248+
// != and == between the same float declaration should NOT warn.
1249+
// < > between the same float declaration SHOULD warn.
1250+
//
1251+
// != and == between eq. expressions that evaluates into float
1252+
// should NOT warn.
1253+
// < > between eq. expressions that evaluates into float
1254+
// should NOT warn.
1255+
//
1256+
const Expr *LHS = BinOp->getLHS()->IgnoreParenImpCasts();
1257+
const Expr *RHS = BinOp->getRHS()->IgnoreParenImpCasts();
1258+
const BinaryOperator::Opcode Op = BinOp->getOpcode();
1259+
const bool OpEqualEQorNE = ((Op == BO_EQ) || (Op == BO_NE));
1260+
1261+
const auto *DeclRef1 = dyn_cast<DeclRefExpr>(LHS);
1262+
const auto *DeclRef2 = dyn_cast<DeclRefExpr>(RHS);
1263+
const auto *FloatLit1 = dyn_cast<FloatingLiteral>(LHS);
1264+
const auto *FloatLit2 = dyn_cast<FloatingLiteral>(RHS);
1265+
1266+
if (DeclRef1 && DeclRef2 &&
1267+
DeclRef1->getType()->hasFloatingRepresentation() &&
1268+
DeclRef2->getType()->hasFloatingRepresentation() &&
1269+
(DeclRef1->getDecl() == DeclRef2->getDecl()) && OpEqualEQorNE) {
1270+
return;
1271+
}
1272+
1273+
if (FloatLit1 && FloatLit2 &&
1274+
FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue()) &&
1275+
OpEqualEQorNE) {
1276+
return;
1277+
}
1278+
12411279
if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
12421280
const Expr *LhsConst = nullptr, *RhsConst = nullptr;
12431281
BinaryOperatorKind MainOpcode{}, SideOpcode{};

‎clang-tools-extra/docs/ReleaseNotes.rst‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,10 @@ Changes in existing checks
156156
<clang-tidy/checks/altera/id-dependent-backward-branch>` check by fixing
157157
crashes from invalid code.
158158

159+
- Improved :doc:`bugprone-branch-clone
160+
<clang-tidy/checks/bugprone/branch-clone>` check to improve detection of
161+
branch clones by now detecting duplicate inner and outer if statements.
162+
159163
- Improved :doc:`bugprone-casting-through-void
160164
<clang-tidy/checks/bugprone/casting-through-void>` check to suggest replacing
161165
the offending code with ``reinterpret_cast``, to more clearly express intent.
@@ -230,6 +234,11 @@ Changes in existing checks
230234
<clang-tidy/checks/misc/definitions-in-headers>` check by rewording the
231235
diagnostic note that suggests adding ``inline``.
232236

237+
- Improved :doc:`misc-redundant-expression
238+
<clang-tidy/checks/misc/redundant-expression>` check by extending the
239+
checker to detect floating point and integer literals in redundant
240+
expressions.
241+
233242
- Improved :doc:`misc-unconventional-assign-operator
234243
<clang-tidy/checks/misc/unconventional-assign-operator>` check to avoid
235244
false positive for C++23 deducing this.

‎clang-tools-extra/docs/clang-tidy/checks/bugprone/branch-clone.rst‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,28 @@ If this is the intended behavior, then there is no reason to use a conditional
3232
statement; otherwise the issue can be solved by fixing the branch that is
3333
handled incorrectly.
3434

35-
The check also detects repeated branches in longer ``if/else if/else`` chains
35+
The check detects repeated branches in longer ``if/else if/else`` chains
3636
where it would be even harder to notice the problem.
3737

38+
The check also detects repeated inner and outer ``if`` statements that may
39+
be a result of a copy-paste error. This check cannot currently detect
40+
identical inner and outer ``if`` statements if code is between the ``if``
41+
conditions. An example is as follows.
42+
43+
.. code-block:: c++
44+
45+
void test_warn_inner_if_1(int x) {
46+
if (x == 1) { // warns, if with identical inner if
47+
if (x == 1) // inner if is here
48+
;
49+
if (x == 1) { // does not warn, cannot detect
50+
int y = x;
51+
if (x == 1)
52+
;
53+
}
54+
}
55+
56+
3857
In ``switch`` statements the check only reports repeated branches when they are
3958
consecutive, because it is relatively common that the ``case:`` labels have
4059
some natural ordering and rearranging them would decrease the readability of

0 commit comments

Comments
 (0)