r196937 - [analyzer] Extend IdenticalExprChecker to check ternary operator results.
Jordan Rose
jordan_rose at apple.com
Tue Dec 10 10:18:06 PST 2013
Author: jrose
Date: Tue Dec 10 12:18:06 2013
New Revision: 196937
URL: http://llvm.org/viewvc/llvm-project?rev=196937&view=rev
Log:
[analyzer] Extend IdenticalExprChecker to check ternary operator results.
Warn if both result expressions of a ternary operator (? :) are the same.
Because only one of them will be executed, this warning will fire even if
the expressions have side effects.
Patch by Anders Rönnholm and Per Viberg!
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
cfe/trunk/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
cfe/trunk/test/Analysis/identical-expressions.cpp
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h?rev=196937&r1=196936&r2=196937&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h Tue Dec 10 12:18:06 2013
@@ -27,7 +27,7 @@
#include <vector>
namespace clang {
-
+class ConditionalOperator;
class AnalysisDeclContext;
class BinaryOperator;
class CompoundStmt;
@@ -211,6 +211,9 @@ public:
/// Assumes the statement has a valid location.
static PathDiagnosticLocation createOperatorLoc(const BinaryOperator *BO,
const SourceManager &SM);
+ static PathDiagnosticLocation createConditionalColonLoc(
+ const ConditionalOperator *CO,
+ const SourceManager &SM);
/// For member expressions, return the location of the '.' or '->'.
/// Assumes the statement has a valid location.
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp?rev=196937&r1=196936&r2=196937&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp Tue Dec 10 12:18:06 2013
@@ -11,7 +11,8 @@
/// \brief This defines IdenticalExprChecker, a check that warns about
/// unintended use of identical expressions.
///
-/// It checks for use of identical expressions with comparison operators.
+/// It checks for use of identical expressions with comparison operators and
+/// inside conditional expressions.
///
//===----------------------------------------------------------------------===//
@@ -26,7 +27,7 @@ using namespace clang;
using namespace ento;
static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
- const Expr *Expr2);
+ const Expr *Expr2, bool IgnoreSideEffects = false);
//===----------------------------------------------------------------------===//
// FindIdenticalExprVisitor - Identify nodes using identical expressions.
//===----------------------------------------------------------------------===//
@@ -38,8 +39,9 @@ public:
explicit FindIdenticalExprVisitor(BugReporter &B, AnalysisDeclContext *A)
: BR(B), AC(A) {}
// FindIdenticalExprVisitor only visits nodes
- // that are binary operators.
+ // that are binary operators or conditional operators.
bool VisitBinaryOperator(const BinaryOperator *B);
+ bool VisitConditionalOperator(const ConditionalOperator *C);
private:
BugReporter &BR;
@@ -118,6 +120,34 @@ bool FindIdenticalExprVisitor::VisitBina
// True is always returned to traverse ALL nodes.
return true;
}
+
+bool FindIdenticalExprVisitor::VisitConditionalOperator(
+ const ConditionalOperator *C) {
+
+ // Check if expressions in conditional expression are identical
+ // from a symbolic point of view.
+
+ if (isIdenticalExpr(AC->getASTContext(), C->getTrueExpr(),
+ C->getFalseExpr(), true)) {
+ PathDiagnosticLocation ELoc =
+ PathDiagnosticLocation::createConditionalColonLoc(
+ C, BR.getSourceManager());
+
+ SourceRange Sr[2];
+ Sr[0] = C->getTrueExpr()->getSourceRange();
+ Sr[1] = C->getFalseExpr()->getSourceRange();
+ BR.EmitBasicReport(
+ AC->getDecl(), "Identical expressions in conditional expression",
+ categories::LogicError,
+ "identical expressions on both sides of ':' in conditional expression",
+ ELoc, Sr);
+ }
+ // We want to visit ALL nodes (expressions in conditional
+ // expressions too) that contains conditional operators,
+ // thus always return true to traverse ALL nodes.
+ return true;
+}
+
/// \brief Determines whether two expression trees are identical regarding
/// operators and symbols.
///
@@ -127,14 +157,14 @@ bool FindIdenticalExprVisitor::VisitBina
/// t*(u + t) and t*u + t*t are not considered identical.
///
static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
- const Expr *Expr2) {
+ const Expr *Expr2, bool IgnoreSideEffects) {
// If Expr1 & Expr2 are of different class then they are not
// identical expression.
if (Expr1->getStmtClass() != Expr2->getStmtClass())
return false;
// If Expr1 has side effects then don't warn even if expressions
// are identical.
- if (Expr1->HasSideEffects(Ctx))
+ if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
return false;
// Is expression is based on macro then don't warn even if
// the expressions are identical.
@@ -146,7 +176,8 @@ static bool isIdenticalExpr(const ASTCon
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))
+ if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2,
+ IgnoreSideEffects))
return false;
++I1;
++I2;
@@ -161,6 +192,7 @@ static bool isIdenticalExpr(const ASTCon
switch (Expr1->getStmtClass()) {
default:
return false;
+ case Stmt::CallExprClass:
case Stmt::ArraySubscriptExprClass:
case Stmt::CStyleCastExprClass:
case Stmt::ImplicitCastExprClass:
@@ -201,7 +233,7 @@ static bool isIdenticalExpr(const ASTCon
const UnaryOperator *UnaryOp2 = dyn_cast<UnaryOperator>(Expr2);
if (UnaryOp1->getOpcode() != UnaryOp2->getOpcode())
return false;
- return !UnaryOp1->isIncrementDecrementOp();
+ return IgnoreSideEffects || !UnaryOp1->isIncrementDecrementOp();
}
}
}
Modified: cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp?rev=196937&r1=196936&r2=196937&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp Tue Dec 10 12:18:06 2013
@@ -610,6 +610,14 @@ PathDiagnosticLocation
}
PathDiagnosticLocation
+ PathDiagnosticLocation::createConditionalColonLoc(
+ const ConditionalOperator *CO,
+ const SourceManager &SM) {
+ return PathDiagnosticLocation(CO->getColonLoc(), SM, SingleLocK);
+}
+
+
+PathDiagnosticLocation
PathDiagnosticLocation::createMemberLoc(const MemberExpr *ME,
const SourceManager &SM) {
return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
Modified: cfe/trunk/test/Analysis/identical-expressions.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/identical-expressions.cpp?rev=196937&r1=196936&r2=196937&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/identical-expressions.cpp (original)
+++ cfe/trunk/test/Analysis/identical-expressions.cpp Tue Dec 10 12:18:06 2013
@@ -2,6 +2,21 @@
/* Only one expected warning per function allowed at the very end. */
+int func(void)
+{
+ return 0;
+}
+
+int func2(void)
+{
+ return 0;
+}
+
+int funcParam(int a)
+{
+ return 0;
+}
+
/* '!=' operator*/
/* '!=' with float */
@@ -295,6 +310,38 @@ int checkNotEqualNestedBinaryOpIntPointe
}
/* end '!=' int* */
+/* '!=' with function*/
+
+int checkNotEqualSameFunction() {
+ unsigned a = 0;
+ unsigned b = 1;
+ int res = (a+func() != a+func()); // no warning
+ return (0);
+}
+
+int checkNotEqualDifferentFunction() {
+ unsigned a = 0;
+ unsigned b = 1;
+ int res = (a+func() != a+func2()); // no warning
+ return (0);
+}
+
+int checkNotEqualSameFunctionSameParam() {
+ unsigned a = 0;
+ unsigned b = 1;
+ int res = (a+funcParam(a) != a+funcParam(a)); // no warning
+ return (0);
+}
+
+int checkNotEqualSameFunctionDifferentParam() {
+ unsigned a = 0;
+ unsigned b = 1;
+ int res = (a+funcParam(a) != a+funcParam(b)); // no warning
+ return (0);
+}
+
+/* end '!=' with function*/
+
/* end '!=' */
@@ -526,6 +573,37 @@ int checkEqualNestedBinaryOpIntCompare3(
return (0);
}
+/* '==' with function*/
+
+int checkEqualSameFunction() {
+ unsigned a = 0;
+ unsigned b = 1;
+ int res = (a+func() == a+func()); // no warning
+ return (0);
+}
+
+int checkEqualDifferentFunction() {
+ unsigned a = 0;
+ unsigned b = 1;
+ int res = (a+func() == a+func2()); // no warning
+ return (0);
+}
+
+int checkEqualSameFunctionSameParam() {
+ unsigned a = 0;
+ unsigned b = 1;
+ int res = (a+funcParam(a) == a+funcParam(a)); // no warning
+ return (0);
+}
+
+int checkEqualSameFunctionDifferentParam() {
+ unsigned a = 0;
+ unsigned b = 1;
+ int res = (a+funcParam(a) == a+funcParam(b)); // no warning
+ return (0);
+}
+
+/* end '==' with function*/
/* end EQ int */
@@ -940,3 +1018,143 @@ int checkGreaterThanNestedBinaryOpIntCom
/* end GT with int */
/* end GT */
+
+
+/* Checking use of identical expressions in conditional operator*/
+
+unsigned test_unsigned(unsigned a) {
+ unsigned b = 1;
+ a = a > 5 ? b : b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+ return a;
+}
+
+void test_signed() {
+ int a = 0;
+ a = a > 5 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_bool(bool a) {
+ a = a > 0 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_float() {
+ float a = 0;
+ float b = 0;
+ a = a > 5 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_unsigned_expr() {
+ unsigned a = 0;
+ unsigned b = 0;
+ a = a > 5 ? a+b : a+b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_signed_expr() {
+ int a = 0;
+ int b = 1;
+ a = a > 5 ? a+b : a+b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_bool_expr(bool a) {
+ bool b = 0;
+ a = a > 0 ? a&&b : a&&b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_unsigned_expr_negative() {
+ unsigned a = 0;
+ unsigned b = 0;
+ a = a > 5 ? a+b : b+a; // no warning
+}
+
+void test_signed_expr_negative() {
+ int a = 0;
+ int b = 1;
+ a = a > 5 ? b+a : a+b; // no warning
+}
+
+void test_bool_expr_negative(bool a) {
+ bool b = 0;
+ a = a > 0 ? a&&b : b&&a; // no warning
+}
+
+void test_float_expr_positive() {
+ float a = 0;
+ float b = 0;
+ a = a > 5 ? a+b : a+b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_expr_positive_func() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+func() : a+func(); // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_expr_negative_func() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+func() : a+func2(); // no warning
+}
+
+void test_expr_positive_funcParam() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+funcParam(b) : a+funcParam(b); // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_expr_negative_funcParam() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+funcParam(a) : a+funcParam(b); // no warning
+}
+
+void test_expr_positive_inc() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a++ : a++; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_expr_negative_inc() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a++ : b++; // no warning
+}
+
+void test_expr_positive_assign() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a=1 : a=1; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_expr_negative_assign() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a=1 : a=2; // no warning
+}
+
+void test_signed_nested_expr() {
+ int a = 0;
+ int b = 1;
+ int c = 3;
+ a = a > 5 ? a+b+(c+a)*(a + b*(c+a)) : a+b+(c+a)*(a + b*(c+a)); // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_signed_nested_expr_negative() {
+ int a = 0;
+ int b = 1;
+ int c = 3;
+ a = a > 5 ? a+b+(c+a)*(a + b*(c+a)) : a+b+(c+a)*(a + b*(a+c)); // no warning
+}
+
+void test_signed_nested_cond_expr_negative() {
+ int a = 0;
+ int b = 1;
+ int c = 3;
+ a = a > 5 ? (b > 5 ? 1 : 4) : (b > 5 ? 2 : 4); // no warning
+}
+
+void test_signed_nested_cond_expr() {
+ int a = 0;
+ int b = 1;
+ int c = 3;
+ a = a > 5 ? (b > 5 ? 1 : 4) : (b > 5 ? 4 : 4); // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
More information about the cfe-commits
mailing list