New check: Comparision of modulo results that are always true/false.

lib/checkother.cpp

@@ -3151,6 +3151,24 @@ void CheckOther::alwaysTrueStringVariableCompareError(const Token *tok, const st
                 "This could be a logic bug.");
 }
 
+//-----------------------------------------------------------------------------
+//-----------------------------------------------------------------------------
+void CheckOther::checkModuloAlwaysTrueFalse()
+{
+    for (const Token* tok = _tokenizer->tokens(); tok; tok = tok->next()) {
+        if (Token::Match(tok, "% %num% ==|!=|<=|<|>|>= %num%") && (!tok->tokAt(4) || !tok->tokAt(4)->isArithmeticalOp())) {
+            if (MathLib::isLessEqual(tok->strAt(1), tok->strAt(3)))
+                moduloAlwaysTrueFalseError(tok, tok->strAt(1));
+        }
+    }
+}
+
+void CheckOther::moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal)
+{
+    reportError(tok, Severity::warning, "moduloAlwaysTrueFalse",
+                "Comparision of modulo result is predetermined, because it is always less than " + maxVal + ".");
+}
+
 //-----------------------------------------------------------------------------
 //-----------------------------------------------------------------------------
 void CheckOther::sizeofsizeof()

lib/checkother.h

@@ -100,6 +100,7 @@ public:
         checkOther.checkComparisonOfBoolWithInt();
         checkOther.checkSwitchCaseFallThrough();
         checkOther.checkAlwaysTrueOrFalseStringCompare();
+        checkOther.checkModuloAlwaysTrueFalse();
 
         checkOther.checkAssignBoolToPointer();
         checkOther.checkBitwiseOnBoolean();
@@ -225,6 +226,9 @@ public:
     /** @brief %Check for suspicious code that compares string literals for equality */
     void checkAlwaysTrueOrFalseStringCompare();
 
+    /** @brief %Check for suspicious usage of modulo (e.g. "if(var % 4 == 4)") */
+    void checkModuloAlwaysTrueFalse();
+
     /** @brief %Check for code that gets never executed, such as duplicate break statements */
     void checkUnreachableCode();
 
@@ -310,6 +314,7 @@ private:
     void SuspiciousSemicolonError(const Token *tok);
     void doubleFreeError(const Token *tok, const std::string &varname);
     void doubleCloseDirError(const Token *tok, const std::string &varname);
+    void moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckOther c(0, settings, errorLogger);
@@ -374,6 +379,7 @@ private:
         c.invalidPrintfArgTypeError_int(0, 1, 'u');
         c.invalidPrintfArgTypeError_float(0, 1, 'f');
         c.cctypefunctionCallError(0, "funname", "value");
+        c.moduloAlwaysTrueFalseError(0, "1");
     }
 
     std::string myName() const {
@@ -431,6 +437,7 @@ private:
                "* using bool in bitwise expression\n"
                "* Suspicious use of ; at the end of 'if/for/while' statement.\n"
                "* incorrect usage of functions from ctype library.\n"
+               "* Comparisions of modulo results that are always true/false.\n"
 
                // optimisations
                "* optimisation: detect post increment/decrement\n";

test/testother.cpp

@@ -112,6 +112,8 @@ private:
 
         TEST_CASE(assignmentInAssert);
 
+        TEST_CASE(modulo);
+
         TEST_CASE(incorrectLogicOperator1);
         TEST_CASE(incorrectLogicOperator2);
         TEST_CASE(incorrectLogicOperator3);
@@ -2649,6 +2651,40 @@ private:
         ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str());
     }
 
+    void modulo() {
+        check("bool f(bool& b1, bool& b2, bool& b3) {\n"
+              "    b1 = a % 5 == 4;\n"
+              "    b2 = a % c == 100000;\n"
+              "    b3 = a % 5 == c;\n"
+              "    return a % 5 == 5-p;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(bool& b1, bool& b2, bool& b3, bool& b4, bool& b5) {\n"
+              "    b1 = a % 5 < 5;\n"
+              "    b2 = a % 5 <= 5;\n"
+              "    b3 = a % 5 == 5;\n"
+              "    b4 = a % 5 != 5;\n"
+              "    b5 = a % 5 >= 5;\n"
+              "    return a % 5 > 5;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Comparision of modulo result is predetermined, because it is always less than 5.\n"
+                      "[test.cpp:3]: (warning) Comparision of modulo result is predetermined, because it is always less than 5.\n"
+                      "[test.cpp:4]: (warning) Comparision of modulo result is predetermined, because it is always less than 5.\n"
+                      "[test.cpp:5]: (warning) Comparision of modulo result is predetermined, because it is always less than 5.\n"
+                      "[test.cpp:6]: (warning) Comparision of modulo result is predetermined, because it is always less than 5.\n"
+                      "[test.cpp:7]: (warning) Comparision of modulo result is predetermined, because it is always less than 5.\n", errout.str());
+
+        check("void f(bool& b1, bool& b2) {\n"
+              "    b1 = bar() % 5 < 889;\n"
+              "    if(x[593] % 5 <= 5)\n"
+              "        b2 = x.a % 5 == 5;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Comparision of modulo result is predetermined, because it is always less than 5.\n"
+                      "[test.cpp:3]: (warning) Comparision of modulo result is predetermined, because it is always less than 5.\n"
+                      "[test.cpp:4]: (warning) Comparision of modulo result is predetermined, because it is always less than 5.\n", errout.str());
+    }
+
     void incorrectLogicOperator1() {
         check("void f(int x) {\n"
               "    if ((x != 1) || (x != 3))\n"