Fixed #2617 (improve check: comparing boolean with '<')

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()

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"

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<Scope>::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.

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.

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"