Symbol database: Refactoring virtual destructors check. Ticket: #1895

lib/checkclass.cpp

@@ -1553,91 +1553,59 @@ void CheckClass::virtualDestructor()
     if (!_settings->inconclusive)
         return;
 
-    const char pattern_classdecl[] = "class %var% : %var%";
+    createSymbolDatabase();
 
-    const Token *derived = _tokenizer->tokens();
+    std::multimap<std::string, SpaceInfo *>::const_iterator i;
-    while ((derived = Token::findmatch(derived, pattern_classdecl)) != NULL)
-    {
-        // Check that the derived class has a non empty destructor..
-        {
-            std::ostringstream destructorPattern;
-            destructorPattern << "~ " << derived->strAt(1) << " ( ) {";
-            const Token *derived_destructor = Token::findmatch(_tokenizer->tokens(), destructorPattern.str().c_str());
 
-            // No destructor..
+    for (i = spaceInfoMMap.begin(); i != spaceInfoMMap.end(); ++i)
-            if (! derived_destructor)
     {
-                derived = derived->next();
+        const SpaceInfo *info = i->second;
+
+        // Skip base classes and namespaces
+        if (info->derivedFrom.empty())
             continue;
-            }
 
-            // Empty destructor..
+        // Find the destructor
-            if (Token::Match(derived_destructor, "~ %var% ( ) { }"))
+        const Func *destructor = info->getDestructor();
-            {
+
-                derived = derived->next();
+        // Check for destructor with implementation
+        if (!destructor || !destructor->hasBody)
             continue;
-            }
-        }
 
+        // Empty destructor
+        if (destructor->tokenDef->tokAt(3)->link() == destructor->tokenDef->tokAt(4))
+            continue;
+
+        const Token *derived = info->classDef;
         const Token *derivedClass = derived->tokAt(1);
 
         // Iterate through each base class...
-        derived = derived->tokAt(3);
+        for (unsigned int j = 0; j < info->derivedFrom.size(); ++j)
-        while (Token::Match(derived, "%var%"))
         {
-            bool isPublic(derived->str() == "public");
+            // Check if base class is public and exists in database
+            if (info->derivedFrom[j].access != Public || !info->derivedFrom[j].spaceInfo)
+                continue;
 
-            // What kind of inheritance is it.. public|protected|private
+            const SpaceInfo *spaceInfo = info->derivedFrom[j].spaceInfo;
-            if (Token::Match(derived, "public|protected|private"))
-                derived = derived->next();
 
             // Name of base class..
-            const std::string baseName = derived->strAt(0);
+            const std::string baseName = spaceInfo->className;
-
-            // Update derived so it's ready for the next loop.
-            do
-            {
-                if (derived->str() == "{")
-                    break;
-
-                if (derived->str() == ",")
-                {
-                    derived = derived->next();
-                    break;
-                }
-
-                derived = derived->next();
-            }
-            while (derived);
-
-            // If not public inheritance, skip checking of this base class..
-            if (! isPublic)
-                continue;
 
             // Find the destructor declaration for the base class.
-            const Token *base = Token::findmatch(_tokenizer->tokens(), (std::string("%any% ~ ") + baseName + " (").c_str());
+            const Func *base_destructor = spaceInfo->getDestructor();
-            while (base && base->str() == "::")
+            const Token *base = 0;
-                base = Token::findmatch(base->next(), (std::string("%any% ~ ") + baseName + " (").c_str());
+            if (base_destructor)
-
+                base = base_destructor->token->tokAt(-2);
-            const Token *reverseTok = base;
-            while (Token::Match(base, "%var%") && base->str() != "virtual")
-                base = base->previous();
 
             // Check that there is a destructor..
-            if (! base)
+            if (!base_destructor)
             {
-                // Is the class declaration available?
+                if (spaceInfo->derivedFrom.empty())
-                base = Token::findmatch(_tokenizer->tokens(), (std::string("class ") + baseName + " {").c_str());
+                    virtualDestructorError(spaceInfo->classDef, baseName, derivedClass->str());
-                if (base)
-                {
-                    virtualDestructorError(base, baseName, derivedClass->str());
-                }
 
                 continue;
             }
-
+            else if (base_destructor->isVirtual)
-            // There is a destructor. Check that it's virtual..
-            else if (base->str() == "virtual")
                 continue;
 
             // TODO: This is just a temporary fix, better solution is needed.
@@ -1652,36 +1620,8 @@ void CheckClass::virtualDestructor()
             // Make sure that the destructor is public (protected or private
             // would not compile if inheritance is used in a way that would
             // cause the bug we are trying to find here.)
-            int indent = 0;
+            if (base_destructor->access == Public)
-            while (reverseTok)
-            {
-                if (reverseTok->str() == "public:")
-                {
                 virtualDestructorError(base, baseName, derivedClass->str());
-                    break;
-                }
-                else if (reverseTok->str() == "protected:" ||
-                         reverseTok->str() == "private:")
-                {
-                    // No bug, protected/private destructor is allowed
-                    break;
-                }
-                else if (reverseTok->str() == "{")
-                {
-                    indent++;
-                    if (indent >= 1)
-                    {
-                        // We have found the start of the class without any sign
-                        // of "public :" so we can assume that the destructor is not
-                        // public and there is no bug in the code we are checking.
-                        break;
-                    }
-                }
-                else if (reverseTok->str() == "}")
-                    indent--;
-
-                reverseTok = reverseTok->previous();
-            }
         }
     }
 }

lib/checkclass.h

@@ -248,6 +248,17 @@ private:
          * @param callstack the function doesn't look into recursive function calls.
          */
         void initializeVarList(const Func &func, std::list<std::string> &callstack);
+
+        const Func *getDestructor() const
+        {
+            std::list<Func>::const_iterator it;
+            for (it = functionList.begin(); it != functionList.end(); ++it)
+            {
+                if (it->type == Func::Destructor)
+                    return &*it;
+            }
+            return 0;
+        }
     };
 
     /** @brief Information about all namespaces/classes/structrues */

test/testclass.cpp

@@ -1418,7 +1418,7 @@ private:
                                " public:\n"
                                " ~B(){int a;}\n"
                                "};\n");
-        ASSERT_EQUALS("[test.cpp:7]: (error) Class AA<double> which is inherited by class B does not have a virtual destructor\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:8]: (error) Class AA<double> which is inherited by class B does not have a virtual destructor\n", errout.str());
     }
 
     void checkUninitVar(const char code[])