diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 7a7667ce3..1d3ff92a8 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2239,6 +2239,126 @@ void CheckOther::misusedScopeObjectError(const Token *tok, const std::string& va "unusedScopedObject", "Instance of '" + varname + "' object is destroyed immediately."); } +//------------------------------------------------------------------------------- +// Comparing functions which are returning value of type bool +//------------------------------------------------------------------------------- + +void CheckOther::checkComparisonOfFuncReturningBool() +{ + if (!_settings->isEnabled("style")) + return; + + if (!_tokenizer->isCPP()) + return; + + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + if (tok->type() == Token::eComparisonOp && tok->str() != "==" && tok->str() != "!=") { + const Token *first_token; + bool first_token_func_of_type_bool = false; + bool second_token_func_of_type_bool = false; + if (Token::simpleMatch(tok->previous(), ")")) { + first_token = tok->previous()->link()->previous(); + } else { + first_token = tok->previous(); + } + + std::string const first_token_name = first_token->str(); + if (first_token->isName()&& isFunction(first_token->str(), _tokenizer->tokens())) { + const Token *fToken = _tokenizer->getFunctionTokenByName(first_token_name.c_str()); + if (fToken &&fToken->previous() && fToken->previous()->str() == "bool") { + first_token_func_of_type_bool = true; + } + } + + Token *second_token = tok->next(); + while (second_token->str()=="!") { + second_token = second_token->next(); + } + std::string const second_token_name = second_token->str(); + if (second_token->isName()&& isFunction(second_token->str(), _tokenizer->tokens())) { + const Token *fToken = _tokenizer->getFunctionTokenByName(second_token_name.c_str()); + if (fToken &&fToken->previous() && fToken->previous()->str() == "bool") { + second_token_func_of_type_bool = true; + } + } + if ((first_token_func_of_type_bool == true) && (second_token_func_of_type_bool == true)) { + comparisonOfTwoFuncsReturningBoolError(first_token->next(), first_token->str(), second_token->str()); + } + if ((first_token_func_of_type_bool == true) && (second_token_func_of_type_bool == false)) { + comparisonOfFuncReturningBoolError(first_token->next(), first_token->str()); + } + if ((first_token_func_of_type_bool == false) && (second_token_func_of_type_bool == true)) { + comparisonOfFuncReturningBoolError(second_token->previous(), second_token->str()); + } + } + } +} + +void CheckOther::comparisonOfFuncReturningBoolError(const Token *tok, const std::string &expression) +{ + reportError(tok, Severity::style, "comparisonOfFuncReturningBoolError", + "Comparison of a function returning boolean value using relational (<, >, <= or >=) operator.\n" + "The return type of function '" + expression + "' is 'bool' " + "and result is of type 'bool'. Comparing 'bool' value using relational (<, >, <= or >=)" + " operator could cause unexpected results."); +} + +void CheckOther::comparisonOfTwoFuncsReturningBoolError(const Token *tok, const std::string &expression1, const std::string &expression2) +{ + reportError(tok, Severity::style, "comparisonOfTwoFuncsReturningBoolError", + "Comparison of two functions returning boolean value using relational (<, >, <= or >=) operator.\n" + "The return type of function '" + expression1 + "' and function '" + expression2 + "' is 'bool' " + "and result is of type 'bool'. Comparing 'bool' value using relational (<, >, <= or >=)" + " operator could cause unexpected results."); +} + +//------------------------------------------------------------------------------- +// Comparison of bool with bool +//------------------------------------------------------------------------------- + +void CheckOther::checkComparisonOfBoolWithBool() +{ + if (!_settings->isEnabled("style")) + return; + + if (!_tokenizer->isCPP()) + return; + + const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase(); + + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + if (tok->type() == Token::eComparisonOp && tok->str() != "==" && tok->str() != "!=") { + bool first_token_bool = false; + bool second_token_bool = false; + + const Token *first_token = tok->previous(); + if (first_token->varId()) { + if (isBool(symbolDatabase->getVariableFromVarId(first_token->varId()))) { + first_token_bool = true; + } + } + const Token *second_token = tok->next(); + if (second_token->varId()) { + if (isBool(symbolDatabase->getVariableFromVarId(second_token->varId()))) { + second_token_bool = true; + } + } + if ((first_token_bool == true) && (second_token_bool == true)) { + comparisonOfBoolWithBoolError(first_token->next(), first_token->str()); + } + } + } +} + +void CheckOther::comparisonOfBoolWithBoolError(const Token *tok, const std::string &expression) +{ + reportError(tok, Severity::style, "comparisonOfBoolWithBoolError", + "Comparison of a variable having boolean value using relational (<, >, <= or >=) operator.\n" + "The variable '" + expression + "' is of type 'bool' " + "and comparing 'bool' value using relational (<, >, <= or >=)" + " operator could cause unexpected results."); +} + //--------------------------------------------------------------------------- //--------------------------------------------------------------------------- void CheckOther::checkIncorrectStringCompare() diff --git a/lib/checkother.h b/lib/checkother.h index 91c7a37a3..9534ba231 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -97,6 +97,8 @@ public: checkOther.checkIncorrectLogicOperator(); checkOther.checkMisusedScopedObject(); + checkOther.checkComparisonOfFuncReturningBool(); + checkOther.checkComparisonOfBoolWithBool(); checkOther.checkMemsetZeroBytes(); checkOther.checkIncorrectStringCompare(); checkOther.checkIncrementBoolean(); @@ -195,6 +197,12 @@ public: /** @brief %Check for objects that are destroyed immediately */ void checkMisusedScopedObject(); + /** @brief %Check for comparison of function returning bool*/ + void checkComparisonOfFuncReturningBool(); + + /** @brief %Check for comparison of variable of type bool*/ + void checkComparisonOfBoolWithBool(); + /** @brief %Check for filling zero bytes with memset() */ void checkMemsetZeroBytes(); @@ -302,6 +310,9 @@ private: void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always); void redundantConditionError(const Token *tok, const std::string &text); void misusedScopeObjectError(const Token *tok, const std::string &varname); + void comparisonOfFuncReturningBoolError(const Token *tok, const std::string &expression); + void comparisonOfTwoFuncsReturningBoolError(const Token *tok, const std::string &expression1, const std::string &expression2); + void comparisonOfBoolWithBoolError(const Token *tok, const std::string &expression); void memsetZeroBytesError(const Token *tok, const std::string &varname); void sizeofForArrayParameterError(const Token *tok); void sizeofForPointerError(const Token *tok, const std::string &varname); @@ -343,6 +354,9 @@ private: c.zerodivError(0); c.mathfunctionCallError(0); c.misusedScopeObjectError(NULL, "varname"); + c.comparisonOfFuncReturningBoolError(0, "func_name"); + c.comparisonOfTwoFuncsReturningBoolError(0, "func_name1", "func_name2"); + c.comparisonOfBoolWithBoolError(0, "var_name"); c.sizeofForArrayParameterError(0); c.sizeofForPointerError(0, "varname"); c.sizeofForNumericParameterError(0); @@ -451,6 +465,8 @@ private: "* using increment on boolean\n" "* comparison of a boolean with a non-zero integer\n" "* comparison of a boolean expression with an integer other than 0 or 1\n" + "* comparison of a function returning boolean value using relational operator\n" + "* comparison of a boolean value with boolean value using relational operator\n" "* suspicious condition (assignment+comparison)\n" "* suspicious condition (runtime comparison of string literals)\n" "* suspicious condition (string literals as boolean)\n" diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 86145bf11..0e5d7f963 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -7553,6 +7553,19 @@ bool Tokenizer::IsScopeNoReturn(const Token *endScopeToken, bool *unknown) //--------------------------------------------------------------------------- +const Token *Tokenizer::getFunctionTokenByName(const char funcname[]) const +{ + std::list::const_iterator scope; + + for (scope = _symbolDatabase->scopeList.begin(); scope != _symbolDatabase->scopeList.end(); ++scope) { + if (scope->type == Scope::eFunction) { + if (scope->classDef->str() == funcname) + return scope->classDef; + } + } + return NULL; +} + bool Tokenizer::isFunctionParameterPassedByValue(const Token *fpar) const { // TODO: If symbol database is available, use it. diff --git a/lib/tokenize.h b/lib/tokenize.h index daa0fb5d0..7387dccf7 100644 --- a/lib/tokenize.h +++ b/lib/tokenize.h @@ -142,6 +142,14 @@ public: */ unsigned int sizeOfType(const Token *type) const; + /** + * Get function token by function name + * @todo better handling of overloaded functions + * @todo only scan parent scopes + * @param funcname function name + */ + const Token *getFunctionTokenByName(const char funcname[]) const; + /** * Try to determine if function parameter is passed by value by looking * at the function declaration. diff --git a/test/testother.cpp b/test/testother.cpp index e7ef4d039..830cd52e4 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -143,6 +143,14 @@ private: TEST_CASE(comparisonOfBoolWithInt4); TEST_CASE(comparisonOfBoolWithInt5); + TEST_CASE(checkComparisonOfFuncReturningBool1); + TEST_CASE(checkComparisonOfFuncReturningBool2); + TEST_CASE(checkComparisonOfFuncReturningBool3); + TEST_CASE(checkComparisonOfFuncReturningBool4); + TEST_CASE(checkComparisonOfFuncReturningBool5); + + TEST_CASE(checkComparisonOfBoolWithBool); + TEST_CASE(duplicateIf); TEST_CASE(duplicateIf1); // ticket 3689 TEST_CASE(duplicateBranch); @@ -3963,6 +3971,140 @@ private: } + void checkComparisonOfFuncReturningBool1() { + check("void f(){\n" + " int temp = 4;\n" + " if(compare1() > compare2()){\n" + " printf(\"foo\");\n" + " }\n" + "}\n" + "bool compare1(int temp){\n" + " if(temp==4){\n" + " return true;\n" + " }\n" + " else\n" + " return false;\n" + "}\n" + "bool compare2(int temp){\n" + " if(temp==4){\n" + " return false;\n" + " }\n" + " else\n" + " return true;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Comparison of two functions returning boolean value using relational (<, >, <= or >=) operator.\n", errout.str()); + } + + void checkComparisonOfFuncReturningBool2() { + check("void f(){\n" + " int temp = 4;\n" + " bool a = true;\n" + " if(compare(temp) > a){\n" + " printf(\"foo\");\n" + " }\n" + "}\n" + "bool compare(int temp){\n" + " if(temp==4){\n" + " return true;\n" + " }\n" + " else\n" + " return false;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) Comparison of a function returning boolean value using relational (<, >, <= or >=) operator.\n", errout.str()); + } + + void checkComparisonOfFuncReturningBool3() { + check("void f(){\n" + " int temp = 4;\n" + " if(compare(temp) > temp){\n" + " printf(\"foo\");\n" + " }\n" + "}\n" + "bool compare(int temp){\n" + " if(temp==4){\n" + " return true;\n" + " }\n" + " else\n" + " return false;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Comparison of a function returning boolean value using relational (<, >, <= or >=) operator.\n", errout.str()); + } + + void checkComparisonOfFuncReturningBool4() { + check("void f(){\n" + " int temp = 4;\n" + " bool b = compare2(6);\n" + " if(compare1(temp)> b){\n" + " printf(\"foo\");\n" + " }\n" + "}\n" + "bool compare1(int temp){\n" + " if(temp==4){\n" + " return true;\n" + " }\n" + " else\n" + " return false;\n" + "}\n" + "bool compare2(int temp){\n" + " if(temp == 5){\n" + " return true;\n" + " }\n" + " else\n" + " return false;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) Comparison of a function returning boolean value using relational (<, >, <= or >=) operator.\n", errout.str()); + } + + void checkComparisonOfFuncReturningBool5() { + check("void f(){\n" + " int temp = 4;\n" + " if(compare1() > !compare2()){\n" + " printf(\"foo\");\n" + " }\n" + "}\n" + "bool compare1(int temp){\n" + " if(temp==4){\n" + " return true;\n" + " }\n" + " else\n" + " return false;\n" + "}\n" + "bool compare2(int temp){\n" + " if(temp==4){\n" + " return false;\n" + " }\n" + " else\n" + " return true;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Comparison of two functions returning boolean value using relational (<, >, <= or >=) operator.\n", errout.str()); + } + + void checkComparisonOfBoolWithBool() { + check("void f(){\n" + " int temp = 4;\n" + " bool b = compare2(6);\n" + " bool a = compare1(4);\n" + " if(b > a){\n" + " printf(\"foo\");\n" + " }\n" + "}\n" + "bool compare1(int temp){\n" + " if(temp==4){\n" + " return true;\n" + " }\n" + " else\n" + " return false;\n" + "}\n" + "bool compare2(int temp){\n" + " if(temp == 5){\n" + " return true;\n" + " }\n" + " else\n" + " return false;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (style) Comparison of a variable having boolean value using relational (<, >, <= or >=) operator.\n", errout.str()); + } + void sizeofForNumericParameter() { check("void f() {\n" " std::cout << sizeof(10) << std::endl;\n"