invalidTestForOverflow: Refactor; move from checkother to checkcondition

lib/checkcondition.cpp

@@ -1014,3 +1014,62 @@ void CheckCondition::alwaysTrueFalseError(const Token *tok, bool knownResult)
                 "knownConditionTrueFalse",
                 "Condition '" + expr + "' is always " + (knownResult ? "true" : "false"));
 }
+
+void CheckCondition::checkInvalidTestForOverflow()
+{
+    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; tok != scope->classEnd; tok = tok->next()) {
+            if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2())
+                continue;
+
+            const Token *calcToken, *exprToken;
+            if (tok->str() == "<" && tok->astOperand1()->str() == "+") {
+                calcToken = tok->astOperand1();
+                exprToken = tok->astOperand2();
+            } else if (tok->str() == ">" && tok->astOperand2()->str() == "+") {
+                calcToken = tok->astOperand2();
+                exprToken = tok->astOperand1();
+            } else
+                continue;
+
+            // Only warn for signed integer overflows and pointer overflows.
+            if (!(calcToken->valueType() && (calcToken->valueType()->pointer || calcToken->valueType()->sign == ValueType::Sign::SIGNED)))
+                continue;
+            if (!(exprToken->valueType() && (exprToken->valueType()->pointer || exprToken->valueType()->sign == ValueType::Sign::SIGNED)))
+                continue;
+
+            const Token *termToken;
+            if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand1(), _settings->library.functionpure))
+                termToken = calcToken->astOperand2();
+            else if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand2(), _settings->library.functionpure))
+                termToken = calcToken->astOperand1();
+            else
+                continue;
+
+            if (!termToken)
+                continue;
+
+            // Only warn when termToken is always positive
+            if (termToken->valueType() && termToken->valueType()->sign == ValueType::Sign::UNSIGNED)
+                invalidTestForOverflow(tok);
+            else if (termToken->isNumber() && MathLib::isPositive(termToken->str()))
+                invalidTestForOverflow(tok);
+        }
+    }
+}
+
+void CheckCondition::invalidTestForOverflow(const Token* tok)
+{
+    std::string errmsg;
+    errmsg = "Invalid test for overflow '" +
+             (tok ? tok->expressionString() : std::string("x + u < x")) +
+             "'. Condition is always false unless there is overflow, and overflow is UB.";
+    reportError(tok, Severity::warning, "invalidTestForOverflow", errmsg);
+}

lib/checkcondition.h

@@ -50,6 +50,7 @@ public:
         checkCondition.clarifyCondition();   // not simplified because ifAssign
         checkCondition.oppositeInnerCondition();
         checkCondition.checkIncorrectLogicOperator();
+        checkCondition.checkInvalidTestForOverflow();
     }
 
     /** @brief Run checks against the simplified token list */
@@ -97,6 +98,9 @@ public:
     /** @brief Condition is always true/false */
     void alwaysTrueFalse();
 
+    /** @brief %Check for invalid test for overflow 'x+100 < x' */
+    void checkInvalidTestForOverflow();
+
 private:
 
     bool isOverlappingCond(const Token * const cond1, const Token * const cond2, const std::set<std::string> &constFunctions) const;
@@ -122,6 +126,8 @@ private:
 
     void alwaysTrueFalseError(const Token *tok, bool knownResult);
 
+    void invalidTestForOverflow(const Token* tok);
+
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckCondition c(0, settings, errorLogger);
 
@@ -136,6 +142,7 @@ private:
         c.moduloAlwaysTrueFalseError(0, "1");
         c.clarifyConditionError(0, true, false);
         c.alwaysTrueFalseError(0, true);
+        c.invalidTestForOverflow(0);
     }
 
     static std::string myName() {
@@ -150,10 +157,11 @@ private:
                "- Detect matching 'if' and 'else if' conditions\n"
                "- Mismatching bitand (a &= 0xf0; a &= 1; => a = 0)\n"
                "- Find dead code which is inaccessible due to the counter-conditions check in nested if statements\n"
-               "- condition that is always true/false\n"
+               "- Condition that is always true/false\n"
-               "- mutual exclusion over || always evaluating to true\n"
+               "- Mutual exclusion over || always evaluating to true\n"
                "- Comparisons of modulo results that are always true/false.\n"
-               "- Known variable values => condition is always true/false\n";
+               "- Known variable values => condition is always true/false\n"
+               "- Invalid test for overflow (for example 'ptr+u < ptr'). Condition is always false unless there is overflow, and overflow is UB.\n";
     }
 };
 /// @}

lib/checkother.cpp

@@ -2455,62 +2455,3 @@ void CheckOther::unusedLabelError(const Token* tok)
     reportError(tok, Severity::style, "unusedLabel",
                 "Label '" + (tok?tok->str():emptyString) + "' is not used.");
 }
-
-void CheckOther::checkInvalidTestForOverflow()
-{
-    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; tok != scope->classEnd; tok = tok->next()) {
-            if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2())
-                continue;
-
-            const Token *calcToken, *exprToken;
-            if (Token::Match(tok, "<|<=") && tok->astOperand1()->str() == "+") {
-                calcToken = tok->astOperand1();
-                exprToken = tok->astOperand2();
-            } else if (Token::Match(tok, ">|>=") && tok->astOperand2()->str() == "+") {
-                calcToken = tok->astOperand2();
-                exprToken = tok->astOperand1();
-            } else
-                continue;
-
-            // Only warn for signed integer overflows and pointer overflows.
-            if (!(calcToken->valueType() && (calcToken->valueType()->pointer || calcToken->valueType()->sign == ValueType::Sign::SIGNED)))
-                continue;
-            if (!(exprToken->valueType() && (exprToken->valueType()->pointer || exprToken->valueType()->sign == ValueType::Sign::SIGNED)))
-                continue;
-
-            const Token *termToken;
-            if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand1(), _settings->library.functionpure))
-                termToken = calcToken->astOperand2();
-            else if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand2(), _settings->library.functionpure))
-                termToken = calcToken->astOperand1();
-            else
-                continue;
-
-            if (!termToken)
-                continue;
-
-            // Only warn when termToken is always positive
-            if (termToken->valueType() && termToken->valueType()->sign == ValueType::Sign::UNSIGNED)
-                invalidTestForOverflow(tok);
-            else if (termToken->isNumber() && MathLib::isPositive(termToken->str()))
-                invalidTestForOverflow(tok);
-        }
-    }
-}
-
-void CheckOther::invalidTestForOverflow(const Token* tok)
-{
-    std::string errmsg;
-    errmsg = "Invalid test for overflow '" +
-             (tok ? tok->expressionString() : std::string("x + u < x")) +
-             "'. Condition is always false unless there is overflow, and overflow is UB.";
-    reportError(tok, Severity::warning, "invalidTestForOverflow", errmsg);
-}

lib/checkother.h

@@ -70,7 +70,6 @@ public:
         checkOther.checkZeroDivision();
         checkOther.checkInterlockedDecrement();
         checkOther.checkUnusedLabel();
-        checkOther.checkInvalidTestForOverflow();
     }
 
     /** @brief Run checks against the simplified token list */
@@ -204,9 +203,6 @@ public:
     /** @brief %Check for unused labels */
     void checkUnusedLabel();
 
-    /** @brief %Check for invalid test for overflow 'x+100 < x' */
-    void checkInvalidTestForOverflow();
-
 private:
     // Error messages..
     void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result);
@@ -255,7 +251,6 @@ private:
     void redundantPointerOpError(const Token* tok, const std::string& varname, bool inconclusive);
     void raceAfterInterlockedDecrementError(const Token* tok);
     void unusedLabelError(const Token* tok);
-    void invalidTestForOverflow(const Token* tok);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckOther c(0, settings, errorLogger);
@@ -310,7 +305,6 @@ private:
         c.commaSeparatedReturnError(0);
         c.redundantPointerOpError(0, "varname", false);
         c.unusedLabelError(0);
-        c.invalidTestForOverflow(0);
     }
 
     static std::string myName() {
@@ -333,7 +327,6 @@ private:
                // warning
                "- either division by zero or useless condition\n"
                "- memset() with a value out of range as the 2nd parameter\n"
-               "- invalid test for overflow\n"
 
                // performance
                "- redundant data copying for const variable\n"

test/testcondition.cpp

@@ -79,6 +79,8 @@ private:
         TEST_CASE(clarifyCondition6);     // #3818
 
         TEST_CASE(alwaysTrue);
+
+        TEST_CASE(checkInvalidTestForOverflow);
     }
 
     void check(const char code[], const char* filename = "test.cpp") {
@@ -1604,6 +1606,23 @@ private:
               "}");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void checkInvalidTestForOverflow() {
+        check("void f(char *p, unsigned int x) {\n"
+              "    assert((p + x) < p);\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow '(p+x)<p'. Condition is always false unless there is overflow, and overflow is UB.\n", errout.str());
+
+        check("void f(signed int x) {\n"
+              "    assert(x + 100 < x);\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow 'x+100<x'. Condition is always false unless there is overflow, and overflow is UB.\n", errout.str());
+
+        check("void f(signed int x) {\n" // unsigned overflow => dont warn
+              "    assert(x + 100U < x);\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestCondition)

test/testother.cpp

@@ -157,8 +157,6 @@ private:
         TEST_CASE(raceAfterInterlockedDecrement);
 
         TEST_CASE(testUnusedLabel);
-
-        TEST_CASE(checkInvalidTestForOverflow);
     }
 
     void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) {
@@ -6019,23 +6017,6 @@ private:
               "}");
         ASSERT_EQUALS("", errout.str());
     }
-
-    void checkInvalidTestForOverflow() {
-        check("void f(char *p, unsigned int x) {\n"
-              "    assert((p + x) < p);\n"
-              "}");
-        ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow '(p+x)<p'. Condition is always false unless there is overflow, and overflow is UB.\n", errout.str());
-
-        check("void f(signed int x) {\n"
-              "    assert(x + 100 < x);\n"
-              "}");
-        ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow 'x+100<x'. Condition is always false unless there is overflow, and overflow is UB.\n", errout.str());
-
-        check("void f(signed int x) {\n" // unsigned overflow => dont warn
-              "    assert(x + 100U < x);\n"
-              "}");
-        ASSERT_EQUALS("", errout.str());
-    }
 };
 
 REGISTER_TEST(TestOther)