Fixed #5049: new check: (warning) Comparison of two identical variables with isgreater(result,result) evaluates always to false.

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+".");
+}
 
 //-----------------------------------------------------------------------------
 //-----------------------------------------------------------------------------

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"

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)