Finally fixed ticket #284 (style check: redundant condition improvement)

http://sourceforge.net/apps/trac/cppcheck/ticket/284

src/checkother.cpp

@@ -73,65 +73,120 @@ void CheckOther::warningRedundantCode()
     // if (p) delete p
     for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next())
     {
-        if (tok->str() != "if")
+        if (! Token::simpleMatch(tok, "if ("))
             continue;
 
-        const char *varname1 = NULL;
+        const char *varname = NULL;
-        const Token *tok2 = NULL;
+        const Token *tok2 = tok->tokAt(2);
 
-        if (Token::Match(tok, "if ( %var% )"))
+        /*
-        {
+         * Possible if-constructions:
-            varname1 = tok->strAt(2);
+         *
-            tok2 = tok->tokAt(4);
+         *   if (0 != var)
-        }
+         *   if (0 != this->var)
-        else if (Token::Match(tok, "if ( %var% != 0 )"))
+         *   if (0 != Foo::var)
-        {
+         *   if (var)
-            varname1 = tok->strAt(2);
+         *   if (this->var)
-            tok2 = tok->tokAt(6);
+         *   if (Foo::var)
+         *   if (var != 0)
+         *   if (this->var != 0)
+         *   if (Foo::var != 0)
+         *
+         **/
+
+        if (Token::simpleMatch(tok2, "0 !=")) {
+            tok2 = tok2->tokAt(2);
         }
 
-        if (varname1 == NULL || tok2 == NULL)
+        if (Token::simpleMatch(tok2, "this .") ||
+            Token::Match(tok2, "%var% ::")) {
+            tok2 = tok2->tokAt(2);
+        }
+
+        if (Token::Match(tok2, "%var%")) {
+            varname = tok2->strAt(0);
+            tok2 = tok2->next();
+        }
+
+        if (Token::simpleMatch(tok2, "!= 0")) {
+            tok2 = tok2->tokAt(2);
+        }
+
+        if (tok2->str() == ")") {
+            tok2 = tok2->next();
+
+        } else {
+            varname = NULL;
+        }
+
+        if (varname == NULL)
             continue;
 
-        bool err = false;
+        bool ifHasBracket = false;
         if (tok2->str() == "{")
         {
             tok2 = tok2->next();
+            ifHasBracket = true;
+        }
 
-            if (Token::Match(tok2, "delete %var% ; }"))
+        bool err = false;
-            {
+        bool funcHasBracket = false;
-                err = (strcmp(tok2->strAt(1), varname1) == 0);
+
-            }
+        /*
-            else if (Token::Match(tok2, "delete [ ] %var% ; }"))
+         * Possible constructions:
-            {
+         *
-                err = (strcmp(tok2->strAt(3), varname1) == 0);
+         * - delete %var%
-            }
+         * - delete [] %var%
-            else if (Token::Match(tok2, "free ( %var% ) ; }"))
+         * - free ( %var )
-            {
+         * - kfree ( %var% )
-                err = (strcmp(tok2->strAt(2), varname1) == 0);
+         *
-            }
+         * Where %var% may be:
-            else if (Token::Match(tok2, "kfree ( %var% ) ; }"))
+         * - just variable name (var)
-            {
+         * - class member (this->var)
-                err = (strcmp(tok2->strAt(2), varname1) == 0);
+         * - static member (Class::var)
+         *
+         **/
+
+        if (Token::Match(tok2, "free|kfree (")) {
+            tok2 = tok2->tokAt(2);
+            funcHasBracket = true;
+
+        } else if (tok2->str() == "delete") {
+
+            tok2 = tok2->next();
+
+            if (Token::simpleMatch(tok2, "[ ]")) {
+                tok2 = tok2->tokAt(2);
             }
         }
-        else
+
-        {
+        if (Token::simpleMatch(tok2, "this .") ||
-            if (Token::Match(tok2, "delete %var% ;"))
+            Token::Match(tok2, "%var% ::")) {
-            {
+            tok2 = tok2->tokAt(2);
-                err = (strcmp(tok2->strAt(1), varname1) == 0);
         }
-            else if (Token::Match(tok2, "delete [ ] %var% ;"))
+
-            {
+        if (Token::Match(tok2, "%var%") && (strcmp(tok2->strAt(0), varname) == 0)) {
-                err = (strcmp(tok2->strAt(3), varname1) == 0);
+            tok2 = tok2->next();
+            err = true;
         }
-            else if (Token::Match(tok2, "free ( %var% ) ;"))
+
-            {
+        if (funcHasBracket) {
-                err = (strcmp(tok2->strAt(2), varname1) == 0);
+            if (tok2->str() != ")") {
+                err = false;
+            } else {
+                tok2 = tok2->next();
             }
-            else if (Token::Match(tok2, "kfree ( %var% ) ;"))
+        }
-            {
+
-                err = (strcmp(tok2->strAt(2), varname1) == 0);
+        if (tok2->str() != ";") {
+            err = false;
+        } else {
+            tok2 = tok2->next();
+        }
+
+        if (ifHasBracket) {
+            if (tok2->str() != "}") {
+                err = false;
             }
         }
 

test/testother.cpp

@@ -163,6 +163,20 @@ private:
               "        delete [] p;\n"
               "}\n");
         ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str());
+
+        check("void foo()\n"
+              "{\n"
+              "    if (0 != this->p)\n"
+              "        delete this->p;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str());
+
+        check("void Foo::deleteInstance()\n"
+              "{\n"
+              "    if (Foo::instance != NULL)\n"
+              "        delete Foo::instance;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str());
     }
 
     void unreachable1()