From 5aaaff46ee88eb34083b1b0600248d77f5eefc12 Mon Sep 17 00:00:00 2001 From: Martin Ettl Date: Thu, 26 Sep 2013 07:07:48 +0200 Subject: [PATCH] Fixed #5049: new check: (warning) Comparison of two identical variables with isgreater(result,result) evaluates always to false. --- lib/checkother.cpp | 47 ++++++++++++++++++++++++++++++++++++++++++++++ lib/checkother.h | 7 +++++++ test/testother.cpp | 36 +++++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index cd6862a0c..3116ac7e6 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3244,6 +3244,53 @@ void CheckOther::suspiciousStringCompareError(const Token* tok, const std::strin "String literal compared with variable '" + var + "'. Did you intend to use strcmp() instead?"); } +//----------------------------------------------------------------------------- +// Check is a comparision of two variables leads to condition, which is +// allways true or false. +// For instance: int a = 1; if(isless(a,a)){...} +// In this case isless(a,a) evaluates allways to false. +// +// Reference: +// - http://www.cplusplus.com/reference/cmath/ +//----------------------------------------------------------------------------- +void CheckOther::checkComparisonFunctionIsAlwaysTrueOrFalse(void) +{ + if (!_settings->isEnabled("warning")) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const std::size_t functions = symbolDatabase->functionScopes.size(); + for (std::size_t i = 0; i < functions; ++i) { + const Scope * scope = symbolDatabase->functionScopes[i]; + for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { + if (tok->isName() && Token::Match(tok, "isgreater|isless|islessgreater|isgreaterequal|islessequal ( %var% , %var% )")) { + const std::string functionName = tok->str(); // store function name + const std::string varNameLeft = tok->tokAt(2)->str(); // get the left variable name + const unsigned int varidLeft = tok->tokAt(2)->varId();// get the left varid + const unsigned int varidRight = tok->tokAt(4)->varId();// get the right varid + // compare varids: if they are not zero but equal + // --> the comparison function is calles with the same variables + if (varidLeft != 0 && varidRight != 0 && varidLeft == varidRight) { + if (functionName == "isgreater" || functionName == "isless" || functionName == "islessgreater") { + // e.g.: isgreater(x,x) --> (x)>(x) --> false + checkComparisonFunctionIsAlwaysTrueOrFalseError(tok,functionName,varNameLeft,false); + } else { // functionName == "isgreaterequal" || functionName == "islessequal" + // e.g.: isgreaterequal(x,x) --> (x)>=(x) --> true + checkComparisonFunctionIsAlwaysTrueOrFalseError(tok,functionName,varNameLeft,true); + } + } + } + } + } +} +void CheckOther::checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &functionName, const std::string &varName, const bool result) +{ + const std::string strResult = result ? "true" : "false"; + reportError(tok, Severity::warning, "comparisonFunctionIsAlwaysTrueOrFalse", + "Comparison of two identical variables with "+functionName+"("+varName+","+varName+") evaluates always to "+strResult+".\n" + "The function "+functionName+" is designed to compare two variables. Calling this function with one variable ("+varName+") " + "for both parameters leads to a statement which is always "+strResult+"."); +} //----------------------------------------------------------------------------- //----------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index 68bebbce8..5d12a7466 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -110,6 +110,7 @@ public: checkOther.checkNegativeBitwiseShift(); checkOther.checkSuspiciousEqualityComparison(); checkOther.checkSleepTimeInterval(); + checkOther.checkComparisonFunctionIsAlwaysTrueOrFalse(); } /** To check the dead code in a program, which is inaccessible due to the counter-conditions check in nested-if statements **/ @@ -265,11 +266,15 @@ public: /** @brief %Check providing too big sleep time intervals on POSIX systems. */ void checkSleepTimeInterval(); + /** @brief %Check for using of comparision functions evaluating always to true or false. */ + void checkComparisonFunctionIsAlwaysTrueOrFalse(void); + private: bool isUnsigned(const Variable *var) const; static bool isSigned(const Variable *var); // Error messages.. + void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result); void checkSleepTimeError(const Token *tok, const std::string &strDim); void checkCastIntToCharAndBackError(const Token *tok, const std::string &strFunctionName); void checkPipeParameterSizeError(const Token *tok, const std::string &strVarName, const std::string &strDim); @@ -352,6 +357,7 @@ private: c.redundantAssignmentError(0, 0, "var", false); // style/warning + c.checkComparisonFunctionIsAlwaysTrueOrFalseError(0,"isless","varName","false"); c.checkCastIntToCharAndBackError(0,"func_name"); c.oppositeInnerConditionError(0); c.cstyleCastError(0); @@ -445,6 +451,7 @@ private: "* Suspicious case labels in switch()\n" "* Suspicious equality comparisons\n" "* mutual exclusion over || always evaluating to true\n" + "* Comparison of values leading always to true or false\n" "* Clarify calculation with parentheses\n" "* suspicious condition (assignment+comparison)\n" "* suspicious condition (runtime comparison of string literals)\n" diff --git a/test/testother.cpp b/test/testother.cpp index 0a82c6567..be021f160 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -179,6 +179,8 @@ private: TEST_CASE(checkSleepTimeIntervall) TEST_CASE(checkCommaSeparatedReturn); + + TEST_CASE(checkComparisonFunctionIsAlwaysTrueOrFalse); } void check(const char code[], const char *filename = NULL, bool experimental = false, bool inconclusive = true, bool posix = false, bool runSimpleChecks=true, Settings* settings = 0) { @@ -6640,6 +6642,40 @@ private: "}", NULL, false, false, false, false); ASSERT_EQUALS("", errout.str()); } + + void checkComparisonFunctionIsAlwaysTrueOrFalse() { + // positive test + check("bool f(int x){\n" + " return isless(x,x);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of two identical variables with isless(x,x) evaluates always to false.\n", errout.str()); + + check("bool f(int x){\n" + " return isgreater(x,x);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of two identical variables with isgreater(x,x) evaluates always to false.\n", errout.str()); + + check("bool f(int x){\n" + " return islessgreater(x,x);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of two identical variables with islessgreater(x,x) evaluates always to false.\n", errout.str()); + + check("bool f(int x){\n" + " return islessequal(x,x);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of two identical variables with islessequal(x,x) evaluates always to true.\n", errout.str()); + + check("bool f(int x){\n" + " return isgreaterequal(x,x);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of two identical variables with isgreaterequal(x,x) evaluates always to true.\n", errout.str()); + + // no warning should be reported for + check("bool f(int x, int y){\n" + " return isgreaterequal(x,y) && islessequal(x,y) && islessgreater(x,y) && isgreater(x,y) && isless(x,y);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther)