[analyzer] Extend IdenticalExprChecker to check the two branches of an if.
This extends the checks for identical expressions to handle identical
statements, and compares the consequent and alternative ("then" and "else")
branches of an if-statement to see if they are identical, treating a single
statement surrounded by braces as equivalent to one without braces.
This does /not/ check subsequent branches in an if/else chain, let alone
branches that are not consecutive. This may improve in a future patch, but
it would certainly take more work.
Patch by Daniel Fahlgren!
llvm-svn: 201701
diff --git a/clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
index aeefc42..6c6f472 100644
--- a/clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -26,8 +26,8 @@
using namespace clang;
using namespace ento;
-static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
- const Expr *Expr2, bool IgnoreSideEffects = false);
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+ const Stmt *Stmt2, bool IgnoreSideEffects = false);
//===----------------------------------------------------------------------===//
// FindIdenticalExprVisitor - Identify nodes using identical expressions.
//===----------------------------------------------------------------------===//
@@ -41,8 +41,10 @@
AnalysisDeclContext *A)
: BR(B), Checker(Checker), AC(A) {}
// FindIdenticalExprVisitor only visits nodes
- // that are binary operators or conditional operators.
+ // that are binary operators, if statements or
+ // conditional operators.
bool VisitBinaryOperator(const BinaryOperator *B);
+ bool VisitIfStmt(const IfStmt *I);
bool VisitConditionalOperator(const ConditionalOperator *C);
private:
@@ -52,6 +54,39 @@
};
} // end anonymous namespace
+bool FindIdenticalExprVisitor::VisitIfStmt(const IfStmt *I) {
+ const Stmt *Stmt1 = I->getThen();
+ const Stmt *Stmt2 = I->getElse();
+
+ if (!Stmt1 || !Stmt2)
+ return true;
+
+ // Special handling for code like:
+ //
+ // if (b) {
+ // i = 1;
+ // } else
+ // i = 1;
+ if (const CompoundStmt *CompStmt = dyn_cast<CompoundStmt>(Stmt1)) {
+ if (CompStmt->size() == 1)
+ Stmt1 = CompStmt->body_back();
+ }
+ if (const CompoundStmt *CompStmt = dyn_cast<CompoundStmt>(Stmt2)) {
+ if (CompStmt->size() == 1)
+ Stmt2 = CompStmt->body_back();
+ }
+
+ if (isIdenticalStmt(AC->getASTContext(), Stmt1, Stmt2, true)) {
+ PathDiagnosticLocation ELoc =
+ PathDiagnosticLocation::createBegin(I, BR.getSourceManager(), AC);
+ BR.EmitBasicReport(AC->getDecl(), Checker,
+ "Identical branches",
+ categories::LogicError,
+ "true and false branches are identical", ELoc);
+ }
+ return true;
+}
+
bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
BinaryOperator::Opcode Op = B->getOpcode();
if (!BinaryOperator::isComparisonOp(Op))
@@ -107,7 +142,7 @@
// No special case with floating-point representation, report as usual.
}
- if (isIdenticalExpr(AC->getASTContext(), B->getLHS(), B->getRHS())) {
+ if (isIdenticalStmt(AC->getASTContext(), B->getLHS(), B->getRHS())) {
PathDiagnosticLocation ELoc =
PathDiagnosticLocation::createOperatorLoc(B, BR.getSourceManager());
StringRef Message;
@@ -131,7 +166,7 @@
// Check if expressions in conditional expression are identical
// from a symbolic point of view.
- if (isIdenticalExpr(AC->getASTContext(), C->getTrueExpr(),
+ if (isIdenticalStmt(AC->getASTContext(), C->getTrueExpr(),
C->getFalseExpr(), true)) {
PathDiagnosticLocation ELoc =
PathDiagnosticLocation::createConditionalColonLoc(
@@ -153,7 +188,7 @@
return true;
}
-/// \brief Determines whether two expression trees are identical regarding
+/// \brief Determines whether two statement trees are identical regarding
/// operators and symbols.
///
/// Exceptions: expressions containing macros or functions with possible side
@@ -161,81 +196,183 @@
/// Limitations: (t + u) and (u + t) are not considered identical.
/// t*(u + t) and t*u + t*t are not considered identical.
///
-static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
- const Expr *Expr2, bool IgnoreSideEffects) {
- // If Expr1 & Expr2 are of different class then they are not
- // identical expression.
- if (Expr1->getStmtClass() != Expr2->getStmtClass())
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+ const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+ if (!Stmt1 && !Stmt2)
+ return true;
return false;
- // If Expr1 has side effects then don't warn even if expressions
- // are identical.
- if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
- return false;
- // If either expression comes from a macro then don't warn even if
- // the expressions are identical.
- if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
- return false;
- // If all children of two expressions are identical, return true.
- Expr::const_child_iterator I1 = Expr1->child_begin();
- Expr::const_child_iterator I2 = Expr2->child_begin();
- while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
- const Expr *Child1 = dyn_cast<Expr>(*I1);
- const Expr *Child2 = dyn_cast<Expr>(*I2);
- if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2,
- IgnoreSideEffects))
- return false;
- ++I1;
- ++I2;
}
- // If there are different number of children in the expressions, return false.
- // (TODO: check if this is a redundant condition.)
- if (I1 != Expr1->child_end())
- return false;
- if (I2 != Expr2->child_end())
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
return false;
- switch (Expr1->getStmtClass()) {
+ const Expr *Expr1 = dyn_cast<Expr>(Stmt1);
+ const Expr *Expr2 = dyn_cast<Expr>(Stmt2);
+
+ if (Expr1 && Expr2) {
+ // If Stmt1 has side effects then don't warn even if expressions
+ // are identical.
+ if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+ // If either expression comes from a macro then don't warn even if
+ // the expressions are identical.
+ if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+ // If all children of two expressions are identical, return true.
+ Expr::const_child_iterator I1 = Expr1->child_begin();
+ Expr::const_child_iterator I2 = Expr2->child_begin();
+ while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+ return false;
+ ++I1;
+ ++I2;
+ }
+ // If there are different number of children in the statements, return
+ // false.
+ if (I1 != Expr1->child_end())
+ return false;
+ if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
default:
return false;
case Stmt::CallExprClass:
case Stmt::ArraySubscriptExprClass:
- case Stmt::CStyleCastExprClass:
case Stmt::ImplicitCastExprClass:
case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
return true;
+ case Stmt::CStyleCastExprClass: {
+ const CStyleCastExpr* CastExpr1 = cast<CStyleCastExpr>(Stmt1);
+ const CStyleCastExpr* CastExpr2 = cast<CStyleCastExpr>(Stmt2);
+
+ return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+ const ReturnStmt *ReturnStmt1 = cast<ReturnStmt>(Stmt1);
+ const ReturnStmt *ReturnStmt2 = cast<ReturnStmt>(Stmt2);
+
+ return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+ ReturnStmt2->getRetValue(), IgnoreSideEffects);
+ }
+ case Stmt::ForStmtClass: {
+ const ForStmt *ForStmt1 = cast<ForStmt>(Stmt1);
+ const ForStmt *ForStmt2 = cast<ForStmt>(Stmt2);
+
+ if (!isIdenticalStmt(Ctx, ForStmt1->getInit(), ForStmt2->getInit(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, ForStmt1->getCond(), ForStmt2->getCond(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, ForStmt1->getInc(), ForStmt2->getInc(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, ForStmt1->getBody(), ForStmt2->getBody(),
+ IgnoreSideEffects))
+ return false;
+ return true;
+ }
+ case Stmt::DoStmtClass: {
+ const DoStmt *DStmt1 = cast<DoStmt>(Stmt1);
+ const DoStmt *DStmt2 = cast<DoStmt>(Stmt2);
+
+ if (!isIdenticalStmt(Ctx, DStmt1->getCond(), DStmt2->getCond(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, DStmt1->getBody(), DStmt2->getBody(),
+ IgnoreSideEffects))
+ return false;
+ return true;
+ }
+ case Stmt::WhileStmtClass: {
+ const WhileStmt *WStmt1 = cast<WhileStmt>(Stmt1);
+ const WhileStmt *WStmt2 = cast<WhileStmt>(Stmt2);
+
+ return isIdenticalStmt(Ctx, WStmt1->getCond(), WStmt2->getCond(),
+ IgnoreSideEffects);
+ }
+ case Stmt::IfStmtClass: {
+ const IfStmt *IStmt1 = cast<IfStmt>(Stmt1);
+ const IfStmt *IStmt2 = cast<IfStmt>(Stmt2);
+
+ if (!isIdenticalStmt(Ctx, IStmt1->getCond(), IStmt2->getCond(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, IStmt1->getThen(), IStmt2->getThen(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, IStmt1->getElse(), IStmt2->getElse(),
+ IgnoreSideEffects))
+ return false;
+ return true;
+ }
+ case Stmt::CompoundStmtClass: {
+ const CompoundStmt *CompStmt1 = cast<CompoundStmt>(Stmt1);
+ const CompoundStmt *CompStmt2 = cast<CompoundStmt>(Stmt2);
+
+ if (CompStmt1->size() != CompStmt2->size())
+ return false;
+
+ CompoundStmt::const_body_iterator I1 = CompStmt1->body_begin();
+ CompoundStmt::const_body_iterator I2 = CompStmt2->body_begin();
+ while (I1 != CompStmt1->body_end() && I2 != CompStmt2->body_end()) {
+ if (!isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+ return false;
+ ++I1;
+ ++I2;
+ }
+
+ return true;
+ }
+ case Stmt::CompoundAssignOperatorClass:
case Stmt::BinaryOperatorClass: {
- const BinaryOperator *BinOp1 = cast<BinaryOperator>(Expr1);
- const BinaryOperator *BinOp2 = cast<BinaryOperator>(Expr2);
+ const BinaryOperator *BinOp1 = cast<BinaryOperator>(Stmt1);
+ const BinaryOperator *BinOp2 = cast<BinaryOperator>(Stmt2);
return BinOp1->getOpcode() == BinOp2->getOpcode();
}
case Stmt::CharacterLiteralClass: {
- const CharacterLiteral *CharLit1 = cast<CharacterLiteral>(Expr1);
- const CharacterLiteral *CharLit2 = cast<CharacterLiteral>(Expr2);
+ const CharacterLiteral *CharLit1 = cast<CharacterLiteral>(Stmt1);
+ const CharacterLiteral *CharLit2 = cast<CharacterLiteral>(Stmt2);
return CharLit1->getValue() == CharLit2->getValue();
}
case Stmt::DeclRefExprClass: {
- const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Expr1);
- const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Expr2);
+ const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Stmt1);
+ const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Stmt2);
return DeclRef1->getDecl() == DeclRef2->getDecl();
}
case Stmt::IntegerLiteralClass: {
- const IntegerLiteral *IntLit1 = cast<IntegerLiteral>(Expr1);
- const IntegerLiteral *IntLit2 = cast<IntegerLiteral>(Expr2);
+ const IntegerLiteral *IntLit1 = cast<IntegerLiteral>(Stmt1);
+ const IntegerLiteral *IntLit2 = cast<IntegerLiteral>(Stmt2);
return IntLit1->getValue() == IntLit2->getValue();
}
case Stmt::FloatingLiteralClass: {
- const FloatingLiteral *FloatLit1 = cast<FloatingLiteral>(Expr1);
- const FloatingLiteral *FloatLit2 = cast<FloatingLiteral>(Expr2);
+ const FloatingLiteral *FloatLit1 = cast<FloatingLiteral>(Stmt1);
+ const FloatingLiteral *FloatLit2 = cast<FloatingLiteral>(Stmt2);
return FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue());
}
+ case Stmt::StringLiteralClass: {
+ const StringLiteral *StringLit1 = cast<StringLiteral>(Stmt1);
+ const StringLiteral *StringLit2 = cast<StringLiteral>(Stmt2);
+ return StringLit1->getString() == StringLit2->getString();
+ }
case Stmt::MemberExprClass: {
- const MemberExpr *MemberExpr1 = cast<MemberExpr>(Expr1);
- const MemberExpr *MemberExpr2 = cast<MemberExpr>(Expr2);
- return MemberExpr1->getMemberDecl() == MemberExpr2->getMemberDecl();
+ const MemberExpr *MemberStmt1 = cast<MemberExpr>(Stmt1);
+ const MemberExpr *MemberStmt2 = cast<MemberExpr>(Stmt2);
+ return MemberStmt1->getMemberDecl() == MemberStmt2->getMemberDecl();
}
case Stmt::UnaryOperatorClass: {
- const UnaryOperator *UnaryOp1 = cast<UnaryOperator>(Expr1);
- const UnaryOperator *UnaryOp2 = cast<UnaryOperator>(Expr2);
+ const UnaryOperator *UnaryOp1 = cast<UnaryOperator>(Stmt1);
+ const UnaryOperator *UnaryOp2 = cast<UnaryOperator>(Stmt2);
return UnaryOp1->getOpcode() == UnaryOp2->getOpcode();
}
}