Improved check: Complain if a variable is modified but not used again

captaingoodnight · Oct 10, 2016 · 1227a3f · 1227a3f
1 parent 04421f5
commit 1227a3f
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 10 deletions.
diff --git a/lib/checkunusedvar.cpp b/lib/checkunusedvar.cpp
@@ -350,6 +350,8 @@ void Variables::modified(unsigned int varid, const Token* tok)
     VariableUsage *usage = find(varid);
 
     if (usage) {
+        if (!usage->_var->isStatic())
+            usage->_read = false;
         usage->_modified = true;
         usage->_lastAccess = tok;
 
@@ -1193,8 +1195,8 @@ void CheckUnusedVar::checkFunctionVariableUsage()
                 unassignedVariableError(usage._var->nameToken(), varname);
 
             // variable has been written but not read
-            else if (!usage._read && !usage._modified)
-                unreadVariableError(usage._lastAccess, varname);
+            else if (!usage._read)
+                unreadVariableError(usage._lastAccess, varname, usage._modified);
 
             // variable has been read but not written
             else if (!usage._write && !usage._allocateMemory && var && !var->isStlType() && !isEmptyType(var->type()))
@@ -1213,9 +1215,12 @@ void CheckUnusedVar::allocatedButUnusedVariableError(const Token *tok, const std
     reportError(tok, Severity::style, "unusedAllocatedMemory", "Variable '" + varname + "' is allocated memory that is never used.", CWE563, false);
 }
 
-void CheckUnusedVar::unreadVariableError(const Token *tok, const std::string &varname)
+void CheckUnusedVar::unreadVariableError(const Token *tok, const std::string &varname, bool modified)
 {
-    reportError(tok, Severity::style, "unreadVariable", "Variable '" + varname + "' is assigned a value that is never used.", CWE563, false);
+    if (modified)
+        reportError(tok, Severity::style, "unreadVariable", "Variable '" + varname + "' is modified but its new value is never used.", CWE563, false);
+    else
+        reportError(tok, Severity::style, "unreadVariable", "Variable '" + varname + "' is assigned a value that is never used.", CWE563, false);
 }
 
 void CheckUnusedVar::unassignedVariableError(const Token *tok, const std::string &varname)

diff --git a/lib/checkunusedvar.h b/lib/checkunusedvar.h
@@ -78,7 +78,7 @@ class CPPCHECKLIB CheckUnusedVar : public Check {
     void unusedStructMemberError(const Token *tok, const std::string &structname, const std::string &varname, bool isUnion = false);
     void unusedVariableError(const Token *tok, const std::string &varname);
     void allocatedButUnusedVariableError(const Token *tok, const std::string &varname);
-    void unreadVariableError(const Token *tok, const std::string &varname);
+    void unreadVariableError(const Token *tok, const std::string &varname, bool modified);
     void unassignedVariableError(const Token *tok, const std::string &varname);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
@@ -87,7 +87,7 @@ class CPPCHECKLIB CheckUnusedVar : public Check {
         // style/warning
         c.unusedVariableError(nullptr, "varname");
         c.allocatedButUnusedVariableError(nullptr, "varname");
-        c.unreadVariableError(nullptr, "varname");
+        c.unreadVariableError(nullptr, "varname", false);
         c.unassignedVariableError(nullptr, "varname");
         c.unusedStructMemberError(nullptr, "structname", "variable");
     }

diff --git a/test/testunusedvar.cpp b/test/testunusedvar.cpp
@@ -1130,15 +1130,17 @@ class TestUnusedVar : public TestFixture {
                               "    int &ii(i);\n"
                               "    ii--;\n"
                               "}");
-        ASSERT_EQUALS("[test.cpp:3]: (style) Variable 'i' is not assigned a value.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:3]: (style) Variable 'i' is not assigned a value.\n"
+                      "[test.cpp:5]: (style) Variable 'ii' is modified but its new value is never used.\n", errout.str());
 
         functionVariableUsage("void foo()\n"
                               "{\n"
                               "    int i;\n"
                               "    int &ii=i;\n"
                               "    ii--;\n"
                               "}");
-        ASSERT_EQUALS("[test.cpp:3]: (style) Variable 'i' is not assigned a value.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:3]: (style) Variable 'i' is not assigned a value.\n"
+                      "[test.cpp:5]: (style) Variable 'ii' is modified but its new value is never used.\n", errout.str());
     }
 
     void localvar8() {
@@ -3409,6 +3411,16 @@ class TestUnusedVar : public TestFixture {
                               "        i += 5;\n"
                               "}");
         ASSERT_EQUALS("", errout.str());
+
+        functionVariableUsage("void foo() {\n"
+                              "    static int x = 0;\n"
+                              "    print(x);\n"
+                              "    if(x > 5)\n"
+                              "        x = 0;\n"
+                              "    else\n"
+                              "        x++;\n"
+                              "}");
+        ASSERT_EQUALS("", errout.str());
     }
 
     void localvarextern() {
@@ -3657,14 +3669,14 @@ class TestUnusedVar : public TestFixture {
     }
 
     void localvardynamic3() {
-        // Ticket #3477 - False positive that 'data' is not assigned a value
+        // Ticket #3467 - False positive that 'data' is not assigned a value
         functionVariableUsage("void foo() {\n"
                               "    int* data = new int[100];\n"
                               "    int* p = data;\n"
                               "    for ( int i = 0; i < 10; ++i )\n"
                               "        p++;\n"
                               "}");
-        ASSERT_EQUALS("", errout.str());
+        ASSERT_EQUALS("[test.cpp:5]: (style) Variable 'p' is modified but its new value is never used.\n", errout.str());
     }
 
     void localvararray1() {