walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

AST: Added experimental new implementations for CheckAssignIf::comparison, CheckOther::checkIncorrectLogicOperator and CheckOther::checkDuplicateExpression

This commit is contained in:
Daniel Marjamäki 2013-11-07 14:38:08 +01:00
parent 43103c84d8
commit b0ce42565e
7 changed files with 268 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
lib/checkassignif.cpp
View File

@ -186,12 +186,56 @@ void CheckAssignIf::mismatchingBitAndError(const Token *tok1, const MathLib::big
}
static void getnumchildren(const Token *tok, std::list<MathLib::bigint> &numchildren)
{
if (tok->astOperand1() && tok->astOperand1()->isNumber())
numchildren.push_back(MathLib::toLongNumber(tok->astOperand1()->str()));
else if (tok->astOperand1() && tok->str() == tok->astOperand1()->str())
getnumchildren(tok->astOperand1(), numchildren);
if (tok->astOperand2() && tok->astOperand2()->isNumber())
numchildren.push_back(MathLib::toLongNumber(tok->astOperand2()->str()));
else if (tok->astOperand2() && tok->str() == tok->astOperand2()->str())
getnumchildren(tok->astOperand2(), numchildren);
}
void CheckAssignIf::comparison()
{
if (!_settings->isEnabled("style"))
return;
if (_settings->ast) {
// Experimental code based on AST
for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) {
if (Token::Match(tok, "==|!=")) {
const Token *expr1 = tok->astOperand1();
const Token *expr2 = tok->astOperand2();
if (!expr1 || !expr2)
continue;
if (expr1->isNumber())
std::swap(expr1,expr2);
if (!expr2->isNumber())
continue;
const MathLib::bigint num2 = MathLib::toLongNumber(expr2->str());
if (num2 < 0)
continue;
if (!Token::Match(expr1,"[&|]"))
continue;
std::list<MathLib::bigint> numbers;
getnumchildren(expr1, numbers);
for (std::list<MathLib::bigint>::const_iterator num = numbers.begin(); num != numbers.end(); ++num) {
const MathLib::bigint num1 = *num;
if (num1 < 0)
continue;
if ((expr1->str() == "&" && (num1 & num2) != num2) ||
(expr1->str() == "|" && (num1 | num2) != num2)) {
const std::string& op(tok->str());
comparisonError(expr1, expr1->str(), num1, op, num2, op=="==" ? false : true);
}
}
}
}
return;
}
for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) {
if (Token::Match(tok, "&|%or% %num% )| ==|!= %num% &&|%oror%|)")) {
const MathLib::bigint num1 = MathLib::toLongNumber(tok->strAt(1));

194
lib/checkother.cpp
View File

@ -33,6 +33,32 @@ namespace {
CheckOther instance;
}
static bool isSameExpression(const Token *tok1, const Token *tok2, const std::set<std::string> &constFunctions)
{
if (tok1 == NULL && tok2 == NULL)
return true;
if (tok1 == NULL || tok2 == NULL)
return false;
if (tok1->str() != tok2->str())
return false;
if (tok1->isExpandedMacro() || tok2->isExpandedMacro())
return false;
if (tok1->isName() && tok1->next()->str() == "(") {
if (!tok1->function() && !Token::Match(tok1->previous(), ".|::") && constFunctions.find(tok1->str()) == constFunctions.end())
return false;
else if (tok1->function() && !tok1->function()->isConst)
return false;
}
if (!isSameExpression(tok1->astOperand1(), tok2->astOperand1(), constFunctions))
return false;
if (!isSameExpression(tok1->astOperand2(), tok2->astOperand2(), constFunctions))
return false;
return true;
}
//----------------------------------------------------------------------------------
// The return value of fgetc(), getc(), ungetc(), getchar() etc. is an integer value.
// If this return value is stored in a character variable and then compared
@ -1225,6 +1251,16 @@ static std::string invertOperatorForOperandSwap(std::string s)
return s;
}
static bool checkRelation(Relation relation, const std::string &value1, const std::string &value2)
{
return (relation == Equal && MathLib::isEqual(value1, value2)) ||
(relation == NotEqual && MathLib::isNotEqual(value1, value2)) ||
(relation == Less && MathLib::isLess(value1, value2)) ||
(relation == LessEqual && MathLib::isLessEqual(value1, value2)) ||
(relation == More && MathLib::isGreater(value1, value2)) ||
(relation == MoreEqual && MathLib::isGreaterEqual(value1, value2));
}
static bool analyzeLogicOperatorCondition(const Condition& c1, const Condition& c2,
bool inv1, bool inv2,
bool varFirst1, bool varFirst2,
@ -1244,12 +1280,7 @@ static bool analyzeLogicOperatorCondition(const Condition& c1, const Condition&
if (!Token::Match(op3Tok, inv2?invertOperatorForOperandSwap(c2.opTokStr).c_str():c2.opTokStr))
return false;
return (relation == Equal && MathLib::isEqual(firstConstant, secondConstant)) ||
(relation == NotEqual && MathLib::isNotEqual(firstConstant, secondConstant)) ||
(relation == Less && MathLib::isLess(firstConstant, secondConstant)) ||
(relation == LessEqual && MathLib::isLessEqual(firstConstant, secondConstant)) ||
(relation == More && MathLib::isGreater(firstConstant, secondConstant)) ||
(relation == MoreEqual && MathLib::isGreaterEqual(firstConstant, secondConstant));
return checkRelation(relation, firstConstant, secondConstant);
}
void CheckOther::checkIncorrectLogicOperator()
@ -1263,6 +1294,135 @@ void CheckOther::checkIncorrectLogicOperator()
const std::size_t functions = symbolDatabase->functionScopes.size();
for (std::size_t ii = 0; ii < functions; ++ii) {
const Scope * scope = symbolDatabase->functionScopes[ii];
if (_settings->ast) {
enum LogicError { AlwaysFalse, AlwaysTrue, FirstTrue, FirstFalse, SecondTrue, SecondFalse };
static const struct {
const char *c1;
const char *logicOp;
const char *c2;
Relation relation;
LogicError error;
} conditions[] = {
{ "!=", "||", "!=", NotEqual , AlwaysTrue }, // (x != 1) || (x != 3) <- always true
{ "==", "&&", "==", NotEqual , AlwaysFalse }, // (x == 1) && (x == 3) <- always false
{ ">" , "||", "<" , Less , AlwaysTrue }, // (x > 3) || (x < 10) <- always true
{ ">=", "||", "<" , LessEqual, AlwaysTrue }, // (x >= 3) || (x < 10) <- always true
{ ">=", "||", "<=", LessEqual, AlwaysTrue }, // (x >= 3) || (x < 10) <- always true
{ ">" , "||", "<=", LessEqual, AlwaysTrue }, // (x > 3) || (x <= 10) <- always true
{ "<" , "&&", ">" , LessEqual, AlwaysFalse }, // (x < 1) && (x > 3) <- always false
{ "<=", "&&", ">" , Less, AlwaysFalse }, // (x <= 1) && (x > 3) <- always false
{ "<" , "&&", ">=", Less, AlwaysFalse }, // (x < 1) && (x >= 3) <- always false
{ ">" , "&&", "==", MoreEqual, AlwaysFalse }, // (x > 5) && (x == 1) <- always false
{ "<" , "&&", "==", LessEqual, AlwaysFalse }, // (x < 1) && (x == 3) <- always false
{ ">=", "&&", "==", More, AlwaysFalse }, // (x >= 5) && (x == 1) <- always false
{ "<=", "&&", "==", Less, AlwaysFalse }, // (x <= 1) && (x == 3) <- always false
{ "==", "||", ">" , More, SecondTrue }, // (x == 4) || (x > 3) <- second expression always true
{ "==", "||", "<" , Less, SecondTrue }, // (x == 4) || (x < 5) <- second expression always true
{ "==", "||", ">=", MoreEqual, SecondTrue }, // (x == 4) || (x >= 3) <- second expression always true
{ "==", "||", "<=", LessEqual, SecondTrue }, // (x == 4) || (x <= 5) <- second expression always true
{ ">" , "||", "!=", MoreEqual, SecondTrue }, // (x > 5) || (x != 1) <- second expression always true
{ "<" , "||", "!=", LessEqual, SecondTrue }, // (x < 1) || (x != 3) <- second expression always true
{ ">=", "||", "!=", More, SecondTrue }, // (x >= 5) || (x != 1) <- second expression always true
{ "<=", "||", "!=", Less, SecondTrue }, // (x <= 1) || (x != 3) <- second expression always true
{ ">" , "&&", "!=", MoreEqual, SecondTrue }, // (x > 5) && (x != 1) <- second expression always true
{ "<" , "&&", "!=", LessEqual, SecondTrue }, // (x < 1) && (x != 3) <- second expression always true
{ ">=", "&&", "!=", More, SecondTrue }, // (x >= 5) && (x != 1) <- second expression always true
{ "<=", "&&", "!=", Less, SecondTrue }, // (x <= 1) && (x != 3) <- second expression always true
{ ">" , "||", ">" , LessEqual, SecondTrue }, // (x > 4) || (x > 5) <- second expression always true
{ "<" , "||", "<" , MoreEqual, SecondTrue }, // (x < 5) || (x < 4) <- second expression always true
{ ">" , "&&", ">" , MoreEqual, SecondTrue }, // (x > 4) && (x > 5) <- second expression always true
{ "<" , "&&", "<" , MoreEqual, SecondTrue }, // (x < 5) && (x < 4) <- second expression always true
{ "==", "&&", "!=", NotEqual, SecondTrue }, // (x == 3) && (x != 4) <- second expression always true
{ "==", "||", "!=", NotEqual, SecondTrue }, // (x == 3) || (x != 4) <- second expression always true
{ "!=", "&&", "==", Equal, AlwaysFalse }, // (x != 3) && (x == 3) <- expression always false
{ "!=", "||", "==", Equal, AlwaysTrue }, // (x != 3) || (x == 3) <- expression always true
};
for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) {
if (Token::Match(tok, "&&|%oror%")) {
if (!tok->astOperand1() || !tok->astOperand1()->astOperand1() || !tok->astOperand1()->astOperand2())
continue;
if (!tok->astOperand2() || !tok->astOperand2()->astOperand1() || !tok->astOperand2()->astOperand2())
continue;
if (!tok->astOperand1()->isComparisonOp() || !tok->astOperand2()->isComparisonOp())
continue;
std::string op1, value1;
const Token *expr1;
if (tok->astOperand1()->astOperand1()->isLiteral()) {
op1 = invertOperatorForOperandSwap(tok->astOperand1()->str());
value1 = tok->astOperand1()->astOperand1()->str();
expr1 = tok->astOperand1()->astOperand2();
} else if (tok->astOperand1()->astOperand2()->isLiteral()) {
op1 = tok->astOperand1()->str();
value1 = tok->astOperand1()->astOperand2()->str();
expr1 = tok->astOperand1()->astOperand1();
} else {
continue;
}
std::string op2, value2;
const Token *expr2;
if (tok->astOperand2()->astOperand1()->isLiteral()) {
op2 = invertOperatorForOperandSwap(tok->astOperand2()->str());
value2 = tok->astOperand2()->astOperand1()->str();
expr2 = tok->astOperand2()->astOperand2();
} else if (tok->astOperand2()->astOperand2()->isLiteral()) {
op2 = tok->astOperand2()->str();
value2 = tok->astOperand2()->astOperand2()->str();
expr2 = tok->astOperand2()->astOperand1();
} else {
continue;
}
const std::set<std::string> constStandardFunctions;
if (!isSameExpression(expr1, expr2, constStandardFunctions))
continue;
for (unsigned int i = 0; i < sizeof(conditions) / sizeof(conditions[0]); i++) {
std::string cond1str;
std::string cond2str;
if (conditions[i].c1 == op1 &&
conditions[i].logicOp == tok->str() &&
conditions[i].c2 == op2 &&
checkRelation(conditions[i].relation, value1, value2)) {
cond1str = (expr1->isName() ? expr1->str() : "EXPR") + " " + op1 + " " + value1;
cond2str = (expr2->isName() ? expr2->str() : "EXPR") + " " + op2 + " " + value2;
}
else if (conditions[i].c1 == op2 &&
conditions[i].logicOp == tok->str() &&
conditions[i].c2 == op1 &&
checkRelation(conditions[i].relation, value2, value1)) {
cond1str = (expr2->isName() ? expr2->str() : "EXPR") + " " + op2 + " " + value2;
cond2str = (expr1->isName() ? expr1->str() : "EXPR") + " " + op1 + " " + value1;
}
else
continue;
if (conditions[i].error == AlwaysFalse || conditions[i].error == AlwaysTrue) {
if (warning) {
const std::string text = cond1str + " " + tok->str() + " " + cond2str;
incorrectLogicOperatorError(tok, text, conditions[i].error == AlwaysTrue);
}
} else {
if (style) {
const std::string text = "If " + cond1str + ", the comparison " + cond2str +
" is always " + ((conditions[i].error == SecondTrue || conditions[i].error == AlwaysTrue) ? "true" : "false") + ".";
redundantConditionError(tok, text);
}
}
break;
}
}
}
continue;
}
for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) {
// Find a pair of comparison expressions with or without parentheses
// with a shared variable and constants and with a logical operator between them.
@ -3132,6 +3292,12 @@ void CheckOther::complexDuplicateExpressionCheck(const std::list<const Function*
}
}
static bool astIsFloat(const Token *tok)
{
// TODO: check function calls, struct members, arrays, etc also
return tok->variable() && Token::Match(tok->variable()->typeStartToken(), "float|double");
}
//---------------------------------------------------------------------------
// check for the same expression on both sides of an operator
// (x == x), (x && x), (x || x)
@ -3154,6 +3320,22 @@ void CheckOther::checkDuplicateExpression()
if (scope->type != Scope::eFunction)
continue;
if (_settings->ast) {
std::set<std::string> constStandardFunctions;
constStandardFunctions.insert("strcmp");
// Experimental implementation
for (const Token *tok = scope->classStart; tok && tok != scope->classEnd; tok = tok->next()) {
if (tok->isOp() && tok->astOperand1() && !Token::Match(tok, "[*=]")) {
if (Token::Match(tok, "==|!=|-") && astIsFloat(tok->astOperand1()))
continue;
if (isSameExpression(tok->astOperand1(), tok->astOperand2(), constStandardFunctions))
duplicateExpressionError(tok, tok, tok->str());
}
}
continue;
}
complexDuplicateExpressionCheck(constFunctions, scope->classStart, "%or%", "");
complexDuplicateExpressionCheck(constFunctions, scope->classStart, "%oror%", "");
complexDuplicateExpressionCheck(constFunctions, scope->classStart, "&", "%oror%|%or%");

4
lib/token.h
View File

@ -669,6 +669,10 @@ public:
return ret;
}
void clearAst() {
_astOperand1 = _astOperand2 = _astParent = NULL;
}
std::string astString(const char *sep = "") const {
std::string ret;
if (_astOperand1)

14
lib/tokenize.cpp
View File

@ -2091,6 +2091,10 @@ bool Tokenizer::tokenize(std::istream &code,
}
}
// Experimental AST handling.
if (_settings->ast)
list.createAst();
return true;
}
//---------------------------------------------------------------------------
@ -3481,6 +3485,10 @@ bool Tokenizer::simplifyTokenList()
// clear the _functionList so it can't contain dead pointers
deleteSymbolDatabase();
// Experimental AST handling.
for (Token *tok = list.front(); tok; tok = tok->next())
tok->clearAst();
simplifyCharAt();
// simplify references
@ -3683,10 +3691,8 @@ bool Tokenizer::simplifyTokenList()
tok->deleteNext();
}
// Experimental AST handling. Only for C code now since
// uninstantiated C++ templates are not handled well. Fix
// TestTokenize::asttemplate
if (_settings->ast && isC())
// Experimental AST handling.
if (_settings->ast)
list.createAst();
if (_settings->terminated())

9
lib/tokenlist.cpp
View File

@ -374,7 +374,8 @@ static void compileBinOp(Token *&tok, void (*f)(Token *&, std::stack<Token*> &),
{
Token *binop = tok;
tok = tok->next();
f(tok,op);
if (tok)
f(tok,op);
// TODO: Should we check if op is empty.
// * Is it better to add assertion that it isn't?
@ -397,6 +398,8 @@ static void compileTerm(Token *& tok, std::stack<Token*> &op)
if (tok->isLiteral()) {
op.push(tok);
tok = tok->next();
} else if (Token::Match(tok, "+|-|~|*|&|!|return")) {
compileUnaryOp(tok, compileExpression, op);
} else if (tok->isName()) {
if (Token::Match(tok->next(), "++|--")) { // post increment / decrement
tok = tok->next();
@ -423,8 +426,6 @@ static void compileTerm(Token *& tok, std::stack<Token*> &op)
}
op.push(name->next());
}
} else if (Token::Match(tok, "+|-|~|*|&|!")) {
compileUnaryOp(tok, compileExpression, op);
} else if (Token::Match(tok, "++|--")) {
if (!op.empty() && op.top()->isOp()) {
// post increment/decrement
@ -604,7 +605,7 @@ static void compileExpression(Token *&tok, std::stack<Token*> &op)
void TokenList::createAst()
{
for (Token *tok = _front; tok; tok = tok ? tok->next() : NULL) {
if (!tok->previous() || Token::Match(tok, "%var% (|[|.|=")) {
if (tok->str() == "return" || !tok->previous() || Token::Match(tok, "%var% (|[|.|=")) {
std::stack<Token *> operands;
compileExpression(tok, operands);
}

30
test/testassignif.cpp
View File

@ -44,6 +44,7 @@ private:
errout.str("");
Settings settings;
settings.ast = true;
settings.addEnabled("style");
// Tokenize..
@ -243,24 +244,12 @@ private:
}
void compare() {
check("void foo(int x)\n"
"{\n"
" if (x & 4 == 3);\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str());
check("void foo(int x)\n"
"{\n"
" if ((x & 4) == 3);\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str());
check("void foo(int x)\n"
"{\n"
" if (x & 4 != 3);\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) != 0x3' is always true.\n", errout.str());
check("void foo(int x)\n"
"{\n"
" if ((x | 4) == 3);\n"
@ -273,24 +262,9 @@ private:
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X | 0x4) != 0x3' is always true.\n", errout.str());
// array
check("void foo(int *x)\n"
"{\n"
" if (x[0] & 4 == 3);\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str());
// struct member
check("void foo(struct X *x)\n"
"{\n"
" if (x->y & 4 == 3);\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str());
// expression
check("void foo(int x)\n"
"{\n"
" if ((x+2) & 4 == 3);\n"
" if ((x & y & 4 & z ) == 3);\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str());
}

21
test/testother.cpp
View File

@ -128,6 +128,7 @@ private:
TEST_CASE(incorrectLogicOperator2);
TEST_CASE(incorrectLogicOperator3);
TEST_CASE(incorrectLogicOperator4);
TEST_CASE(incorrectLogicOperator5);
TEST_CASE(secondAlwaysTrueFalseWhenFirstTrueError);
TEST_CASE(incorrectLogicOp_condSwapping);
TEST_CASE(sameExpression);
@ -200,6 +201,7 @@ private:
if (!settings) {
static Settings _settings;
settings = &_settings;
_settings.ast = true;
}
settings->addEnabled("style");
settings->addEnabled("warning");
@ -443,8 +445,8 @@ private:
void zeroDiv7() {
check("void f() {\n"
" int a = 1/2*3/0;\n"
" int b = 1/2*3%0;\n"
" int a = x/2*3/0;\n"
" int b = y/2*3%0;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (error) Division by zero.\n"
"[test.cpp:3]: (error) Division by zero.\n", errout.str());
@ -4031,6 +4033,13 @@ private:
ASSERT_EQUALS("", errout.str());
}
void incorrectLogicOperator5() {
check("void f(int x) {\n"
" if (x+3 > 2 || x+3 < 10) {}\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: EXPR > 2 || EXPR < 10.\n", errout.str());
}
void secondAlwaysTrueFalseWhenFirstTrueError() {
check("void f(int x) {\n"
" if (x > 5 && x != 1)\n"
@ -4320,7 +4329,7 @@ private:
// clarify conditions with bitwise operator and comparison
void clarifyCondition2() {
check("void f() {\n"
" if (x & 2 == 2) {}\n"
" if (x & 3 == 2) {}\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Suspicious condition (bitwise operator + comparison); Clarify expression with parentheses.\n", errout.str());
@ -4691,9 +4700,9 @@ private:
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
check("void foo() {\n"
" if (x!=2 || x!=3 || x!=2) {}\n"
" if (x!=2 || y!=3 || x!=2) {}\n"
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
TODO_ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", "", errout.str());
check("void foo() {\n"
" if (a && b || a && b) {}\n"
@ -4708,7 +4717,7 @@ private:
check("void foo() {\n"
" if (a && b | b && c) {}\n"
"}");
ASSERT_EQUALS("", errout.str());
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '|'.\n", errout.str());
check("void foo() {\n"
" if ((a + b) | (a + b)) {}\n"