From 83c460fc5606e5746991791b1eba1923eaa3871f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sat, 7 Sep 2013 07:40:10 +0200 Subject: [PATCH] Fixed #5017 (New check: division by zero, otherwise condition is redundant) --- lib/checkother.cpp | 68 ++++++++++++++++++++++++++++++++++++++++++++++ lib/checkother.h | 9 ++++++ test/testother.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 2ce209449..afa89c21d 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2155,6 +2155,74 @@ void CheckOther::zerodivError(const Token *tok) reportError(tok, Severity::error, "zerodiv", "Division by zero."); } +//--------------------------------------------------------------------------- +void CheckOther::checkZeroDivisionOrUselessCondition() +{ + if (!_settings->isEnabled("warning")) + return; + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const std::size_t numberOfFunctions = symbolDatabase->functionScopes.size(); + for (std::size_t functionIndex = 0; functionIndex < numberOfFunctions; ++functionIndex) { + const Scope * scope = symbolDatabase->functionScopes[functionIndex]; + for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { + if (Token::Match(tok, "[/%] %var% !!.")) { + const unsigned int varid = tok->next()->varId(); + const Variable *var = tok->next()->variable(); + if (!var) + continue; + bool isVarUnsigned = var->typeEndToken()->isUnsigned(); + for (const Token *typetok = var->typeStartToken(); typetok != var->typeEndToken(); typetok = typetok->next()) { + if (typetok->isUnsigned()) { + isVarUnsigned = true; + break; + } + } + for (const Token *tok2 = tok->tokAt(2); tok2; tok2 = tok2->next()) { + if (tok2->varId() == varid) + break; + if (tok2->str() == "}") + break; + if (Token::Match(tok2, "%var% (") && tok2->str() != "if" && (var->isGlobal() || !tok2->function())) + break; + if (isVarUnsigned && Token::Match(tok2, "(|%oror%|&& 0 < %varid% &&|%oror%|)", varid)) { + zerodivcondError(tok2,tok); + continue; + } + if (isVarUnsigned && Token::Match(tok2, "(|%oror%|&& 1 <= %varid% &&|%oror%|)", varid)) { + zerodivcondError(tok2,tok); + continue; + } + if (Token::Match(tok2, "(|%oror%|&& !| %varid% &&|%oror%|)", varid)) { + zerodivcondError(tok2,tok); + continue; + } + } + } + } + } +} + +void CheckOther::zerodivcondError(const Token *tokcond, const Token *tokdiv) +{ + std::list callstack; + while (Token::Match(tokcond, "(|%oror%|&&")) + tokcond = tokcond->next(); + if (tokcond && tokdiv) { + callstack.push_back(tokcond); + callstack.push_back(tokdiv); + } + std::string condition; + if (Token::Match(tokcond, "%num% <|<=")) { + condition = tokcond->strAt(2) + ((tokcond->strAt(1) == "<") ? ">" : ">=") + tokcond->str(); + } else if (tokcond) { + if (tokcond->str() == "!") + condition = tokcond->next()->str() + "==0"; + else + condition = tokcond->str() + "!=0"; + } + const std::string linenr(MathLib::longToString(tokdiv ? tokdiv->linenr() : 0)); + reportError(callstack, Severity::warning, "zerodivcond", "Either the condition '"+condition+"' is useless or there is division by zero at line " + linenr + "."); +} //--------------------------------------------------------------------------- //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index 4fd9d579b..68bebbce8 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -90,6 +90,7 @@ public: checkOther.invalidFunctionUsage(); checkOther.checkZeroDivision(); + checkOther.checkZeroDivisionOrUselessCondition(); checkOther.checkMathFunctions(); checkOther.checkCCTypeFunctions(); @@ -163,6 +164,9 @@ public: /** @brief %Check zero division*/ void checkZeroDivision(); + /** @brief %Check zero division / useless condition */ + void checkZeroDivisionOrUselessCondition(); + /** @brief Check for NaN (not-a-number) in an arithmetic expression */ void checkNanInArithmeticExpression(); @@ -286,6 +290,7 @@ private: void variableScopeError(const Token *tok, const std::string &varname); void strPlusCharError(const Token *tok); void zerodivError(const Token *tok); + void zerodivcondError(const Token *tokcond, const Token *tokdiv); void nanInArithmeticExpressionError(const Token *tok); void mathfunctionCallError(const Token *tok, const unsigned int numParam = 1); void cctypefunctionCallError(const Token *tok, const std::string &functionName, const std::string &value); @@ -332,6 +337,7 @@ private: c.sprintfOverlappingDataError(0, "varname"); c.udivError(0, false); c.zerodivError(0); + c.zerodivcondError(0,0); c.mathfunctionCallError(0); c.misusedScopeObjectError(NULL, "varname"); c.doubleFreeError(0, "varname"); @@ -411,6 +417,9 @@ private: "* cast the return values of getc(),fgetc() and getchar() to character and compare it to EOF\n" "* provide too big sleep times on POSIX systems\n" + // warning + "* either division by zero or useless condition\n" + //performance "* redundant data copying for const variable\n" "* subsequent assignment or copying to a variable or buffer\n" diff --git a/test/testother.cpp b/test/testother.cpp index d89ce1cd9..99f6fbdfc 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -44,6 +44,8 @@ private: TEST_CASE(zeroDiv6); TEST_CASE(zeroDiv7); // #4930 + TEST_CASE(zeroDivCond); // division by zero / useless condition + TEST_CASE(nanInArithmeticExpression); TEST_CASE(sprintf1); // Dangerous usage of sprintf @@ -428,6 +430,70 @@ private: "[test.cpp:3]: (error) Division by zero.\n", errout.str()); } + void zeroDivCond() { + check("void f(unsigned int x) {\n" + " int y = 17 / x;\n" + " if (x > 0) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Either the condition 'x>0' is useless or there is division by zero at line 2.\n", errout.str()); + + check("void f(unsigned int x) {\n" + " int y = 17 / x;\n" + " if (x >= 1) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Either the condition 'x>=1' is useless or there is division by zero at line 2.\n", errout.str()); + + check("void f(int x) {\n" + " int y = 17 / x;\n" + " if (x == 0) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Either the condition 'x==0' is useless or there is division by zero at line 2.\n", errout.str()); + + check("void f(unsigned int x) {\n" + " int y = 17 / x;\n" + " if (x != 0) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Either the condition 'x!=0' is useless or there is division by zero at line 2.\n", errout.str()); + + // avoid false positives when variable is changed after division + check("void f() {\n" + " unsigned int x = do_something();\n" + " int y = 17 / x;\n" + " x = some+calculation;\n" + " if (x != 0) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + { + // function is called that might modify global variable + check("void do_something();\n" + "int x;\n" + "void f() {\n" + " int y = 17 / x;\n" + " do_something();\n" + " if (x != 0) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // function is called. but don't care, variable is local + check("void do_something();\n" + "void f() {\n" + " int x = some + calculation;\n" + " int y = 17 / x;\n" + " do_something();\n" + " if (x != 0) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:4]: (warning) Either the condition 'x!=0' is useless or there is division by zero at line 4.\n", errout.str()); + } + + check("int x;\n" + "void f() {\n" + " int y = 17 / x;\n" + " while (y || x == 0) { x--; }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + void nanInArithmeticExpression() { check("void f()\n" "{\n"