Fixed #5017 (New check: division by zero, otherwise condition is redundant)

lib/checkother.cpp

@@ -2155,6 +2155,74 @@ void CheckOther::zerodivError(const Token *tok)
     reportError(tok, Severity::error, "zerodiv", "Division by zero.");
 }
 
+//---------------------------------------------------------------------------
+void CheckOther::checkZeroDivisionOrUselessCondition()
+{
+    if (!_settings->isEnabled("warning"))
+        return;
+    const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
+    const std::size_t numberOfFunctions = symbolDatabase->functionScopes.size();
+    for (std::size_t functionIndex = 0; functionIndex < numberOfFunctions; ++functionIndex) {
+        const Scope * scope = symbolDatabase->functionScopes[functionIndex];
+        for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) {
+            if (Token::Match(tok, "[/%] %var% !!.")) {
+                const unsigned int varid = tok->next()->varId();
+                const Variable *var = tok->next()->variable();
+                if (!var)
+                    continue;
+                bool isVarUnsigned = var->typeEndToken()->isUnsigned();
+                for (const Token *typetok = var->typeStartToken(); typetok != var->typeEndToken(); typetok = typetok->next()) {
+                    if (typetok->isUnsigned()) {
+                        isVarUnsigned = true;
+                        break;
+                    }
+                }
+                for (const Token *tok2 = tok->tokAt(2); tok2; tok2 = tok2->next()) {
+                    if (tok2->varId() == varid)
+                        break;
+                    if (tok2->str() == "}")
+                        break;
+                    if (Token::Match(tok2, "%var% (") && tok2->str() != "if" && (var->isGlobal() || !tok2->function()))
+                        break;
+                    if (isVarUnsigned && Token::Match(tok2, "(|%oror%|&& 0 < %varid% &&|%oror%|)", varid)) {
+                        zerodivcondError(tok2,tok);
+                        continue;
+                    }
+                    if (isVarUnsigned && Token::Match(tok2, "(|%oror%|&& 1 <= %varid% &&|%oror%|)", varid)) {
+                        zerodivcondError(tok2,tok);
+                        continue;
+                    }
+                    if (Token::Match(tok2, "(|%oror%|&& !| %varid% &&|%oror%|)", varid)) {
+                        zerodivcondError(tok2,tok);
+                        continue;
+                    }
+                }
+            }
+        }
+    }
+}
+
+void CheckOther::zerodivcondError(const Token *tokcond, const Token *tokdiv)
+{
+    std::list<const Token *> callstack;
+    while (Token::Match(tokcond, "(|%oror%|&&"))
+        tokcond = tokcond->next();
+    if (tokcond && tokdiv) {
+        callstack.push_back(tokcond);
+        callstack.push_back(tokdiv);
+    }
+    std::string condition;
+    if (Token::Match(tokcond, "%num% <|<=")) {
+        condition = tokcond->strAt(2) + ((tokcond->strAt(1) == "<") ? ">" : ">=") + tokcond->str();
+    } else if (tokcond) {
+        if (tokcond->str() == "!")
+            condition = tokcond->next()->str() + "==0";
+        else
+            condition = tokcond->str() + "!=0";
+    }
+    const std::string linenr(MathLib::longToString(tokdiv ? tokdiv->linenr() : 0));
+    reportError(callstack, Severity::warning, "zerodivcond", "Either the condition '"+condition+"' is useless or there is division by zero at line " + linenr + ".");
+}
 //---------------------------------------------------------------------------
 //---------------------------------------------------------------------------
 

lib/checkother.h

@@ -90,6 +90,7 @@ public:
 
         checkOther.invalidFunctionUsage();
         checkOther.checkZeroDivision();
+        checkOther.checkZeroDivisionOrUselessCondition();
         checkOther.checkMathFunctions();
         checkOther.checkCCTypeFunctions();
 
@@ -163,6 +164,9 @@ public:
     /** @brief %Check zero division*/
     void checkZeroDivision();
 
+    /** @brief %Check zero division / useless condition */
+    void checkZeroDivisionOrUselessCondition();
+
     /** @brief Check for NaN (not-a-number) in an arithmetic expression */
     void checkNanInArithmeticExpression();
 
@@ -286,6 +290,7 @@ private:
     void variableScopeError(const Token *tok, const std::string &varname);
     void strPlusCharError(const Token *tok);
     void zerodivError(const Token *tok);
+    void zerodivcondError(const Token *tokcond, const Token *tokdiv);
     void nanInArithmeticExpressionError(const Token *tok);
     void mathfunctionCallError(const Token *tok, const unsigned int numParam = 1);
     void cctypefunctionCallError(const Token *tok, const std::string &functionName, const std::string &value);
@@ -332,6 +337,7 @@ private:
         c.sprintfOverlappingDataError(0, "varname");
         c.udivError(0, false);
         c.zerodivError(0);
+        c.zerodivcondError(0,0);
         c.mathfunctionCallError(0);
         c.misusedScopeObjectError(NULL, "varname");
         c.doubleFreeError(0, "varname");
@@ -411,6 +417,9 @@ private:
                "* cast the return values of getc(),fgetc() and getchar() to character and compare it to EOF\n"
                "* provide too big sleep times on POSIX systems\n"
 
+               // warning
+               "* either division by zero or useless condition\n"
+
                //performance
                "* redundant data copying for const variable\n"
                "* subsequent assignment or copying to a variable or buffer\n"

test/testother.cpp

@@ -44,6 +44,8 @@ private:
         TEST_CASE(zeroDiv6);
         TEST_CASE(zeroDiv7);  // #4930
 
+        TEST_CASE(zeroDivCond); // division by zero / useless condition
+
         TEST_CASE(nanInArithmeticExpression);
 
         TEST_CASE(sprintf1);        // Dangerous usage of sprintf
@@ -428,6 +430,70 @@ private:
                       "[test.cpp:3]: (error) Division by zero.\n", errout.str());
     }
 
+    void zeroDivCond() {
+        check("void f(unsigned int x) {\n"
+              "  int y = 17 / x;\n"
+              "  if (x > 0) {}\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Either the condition 'x>0' is useless or there is division by zero at line 2.\n", errout.str());
+
+        check("void f(unsigned int x) {\n"
+              "  int y = 17 / x;\n"
+              "  if (x >= 1) {}\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Either the condition 'x>=1' is useless or there is division by zero at line 2.\n", errout.str());
+
+        check("void f(int x) {\n"
+              "  int y = 17 / x;\n"
+              "  if (x == 0) {}\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Either the condition 'x==0' is useless or there is division by zero at line 2.\n", errout.str());
+
+        check("void f(unsigned int x) {\n"
+              "  int y = 17 / x;\n"
+              "  if (x != 0) {}\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Either the condition 'x!=0' is useless or there is division by zero at line 2.\n", errout.str());
+
+        // avoid false positives when variable is changed after division
+        check("void f() {\n"
+              "  unsigned int x = do_something();\n"
+              "  int y = 17 / x;\n"
+              "  x = some+calculation;\n"
+              "  if (x != 0) {}\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        {
+            // function is called that might modify global variable
+            check("void do_something();\n"
+                  "int x;\n"
+                  "void f() {\n"
+                  "  int y = 17 / x;\n"
+                  "  do_something();\n"
+                  "  if (x != 0) {}\n"
+                  "}");
+            ASSERT_EQUALS("", errout.str());
+
+            // function is called. but don't care, variable is local
+            check("void do_something();\n"
+                  "void f() {\n"
+                  "  int x = some + calculation;\n"
+                  "  int y = 17 / x;\n"
+                  "  do_something();\n"
+                  "  if (x != 0) {}\n"
+                  "}");
+            ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:4]: (warning) Either the condition 'x!=0' is useless or there is division by zero at line 4.\n", errout.str());
+        }
+
+        check("int x;\n"
+              "void f() {\n"
+              "  int y = 17 / x;\n"
+              "  while (y || x == 0) { x--; }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void nanInArithmeticExpression() {
         check("void f()\n"
               "{\n"