Fixed cppcheck-opensource#5807 (non virtual dtor in virtual class)

IOBYTE · danmar · commit 1f09cb0c302b · 2014-06-14T12:55:20.000+02:00
diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp
@@ -1381,14 +1381,31 @@ void CheckClass::virtualDestructor()
     // * base class doesn't have virtual destructor
     // * derived class has non-empty destructor
     // * base class is deleted
+    // unless inconclusive in which case:
+    // * base class has virtual members but doesn't have virtual destructor
+
+    std::list<const Function *> inconclusive_errors;
 
     const std::size_t classes = symbolDatabase->classAndStructScopes.size();
     for (std::size_t i = 0; i < classes; ++i) {
         const Scope * scope = symbolDatabase->classAndStructScopes[i];
 
-        // Skip base classes
-        if (scope->definedType->derivedFrom.empty())
+        // Skip base classes (unless inconclusive)
+        if (scope->definedType->derivedFrom.empty()) {
+            if (_settings->inconclusive) {
+                const Function *destructor = scope->getDestructor();
+                if (destructor && !destructor->isVirtual) {
+                    std::list<Function>::const_iterator func;
+                    for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
+                        if (func->isVirtual) {
+                            inconclusive_errors.push_back(destructor);
+                            break;
+                        }
+                    }
+                }
+            }
             continue;
+        }
 
         // Find the destructor
         const Function *destructor = scope->getDestructor();
@@ -1466,8 +1483,13 @@ void CheckClass::virtualDestructor()
 
                 // Check that there is a destructor..
                 if (!base_destructor) {
-                    if (derivedFrom->derivedFrom.empty())
-                        virtualDestructorError(derivedFrom->classDef, derivedFrom->name(), derivedClass->str());
+                    if (derivedFrom->derivedFrom.empty()) {
+                        virtualDestructorError(derivedFrom->classDef, derivedFrom->name(), derivedClass->str(), false);
+                        // check for duplicate error and remove if if found
+                        std::list<const Function *>::iterator found = find(inconclusive_errors.begin(), inconclusive_errors.end(), base_destructor);
+                        if (found != inconclusive_errors.end())
+                            inconclusive_errors.erase(found);
+                    }
                 } else if (!base_destructor->isVirtual) {
                     // TODO: This is just a temporary fix, better solution is needed.
                     // Skip situations where base class has base classes of its own, because
@@ -1479,22 +1501,33 @@ 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.)
-                        if (base_destructor->access == Public)
-                            virtualDestructorError(base, derivedFrom->name(), derivedClass->str());
+                        if (base_destructor->access == Public) {
+                            virtualDestructorError(base, derivedFrom->name(), derivedClass->str(), false);
+                            // check for duplicate error and remove if if found
+                            std::list<const Function *>::iterator found = find(inconclusive_errors.begin(), inconclusive_errors.end(), base_destructor);
+                            if (found != inconclusive_errors.end())
+                                inconclusive_errors.erase(found);
+                        }
                     }
                 }
             }
         }
     }
+
+    for (std::list<const Function *>::const_iterator i = inconclusive_errors.begin(); i != inconclusive_errors.end(); ++i)
+        virtualDestructorError((*i)->tokenDef, (*i)->name(), "", true);
 }
 
-void CheckClass::virtualDestructorError(const Token *tok, const std::string &Base, const std::string &Derived)
+void CheckClass::virtualDestructorError(const Token *tok, const std::string &Base, const std::string &Derived, bool inconclusive)
 {
-    reportError(tok, Severity::error, "virtualDestructor", "Class '" + Base + "' which is inherited by class '" + Derived + "' does not have a virtual destructor.\n"
-                "Class '" + Base + "' which is inherited by class '" + Derived + "' does not have a virtual destructor. "
-                "If you destroy instances of the derived class by deleting a pointer that points to the base class, only "
-                "the destructor of the base class is executed. Thus, dynamic memory that is managed by the derived class "
-                "could leak. This can be avoided by adding a virtual destructor to the base class.");
+    if (inconclusive)
+        reportError(tok, Severity::warning, "virtualDestructor", "Class '" + Base + "' which has virtual members does not have a virtual destructor.", true);
+    else
+        reportError(tok, Severity::error, "virtualDestructor", "Class '" + Base + "' which is inherited by class '" + Derived + "' does not have a virtual destructor.\n"
+                    "Class '" + Base + "' which is inherited by class '" + Derived + "' does not have a virtual destructor. "
+                    "If you destroy instances of the derived class by deleting a pointer that points to the base class, only "
+                    "the destructor of the base class is executed. Thus, dynamic memory that is managed by the derived class "
+                    "could leak. This can be avoided by adding a virtual destructor to the base class.");
 }
 
 //---------------------------------------------------------------------------
diff --git a/lib/checkclass.h b/lib/checkclass.h
@@ -142,7 +142,7 @@ class CPPCHECKLIB CheckClass : public Check {
     void mallocOnClassError(const Token* tok, const std::string &memfunc, const Token* classTok, const std::string &classname);
     void mallocOnClassWarning(const Token* tok, const std::string &memfunc, const Token* classTok);
     void operatorEqReturnError(const Token *tok, const std::string &className);
-    void virtualDestructorError(const Token *tok, const std::string &Base, const std::string &Derived);
+    void virtualDestructorError(const Token *tok, const std::string &Base, const std::string &Derived, bool inconclusive);
     void thisSubtractionError(const Token *tok);
     void operatorEqRetRefThisError(const Token *tok);
     void operatorEqToSelfError(const Token *tok);
@@ -166,7 +166,7 @@ class CPPCHECKLIB CheckClass : public Check {
         c.mallocOnClassWarning(0, "malloc", 0);
         c.mallocOnClassError(0, "malloc", 0, "std::string");
         c.operatorEqReturnError(0, "class");
-        c.virtualDestructorError(0, "Base", "Derived");
+        c.virtualDestructorError(0, "Base", "Derived", false);
         c.thisSubtractionError(0);
         c.operatorEqRetRefThisError(0);
         c.operatorEqToSelfError(0);
diff --git a/test/testclass.cpp b/test/testclass.cpp
@@ -43,6 +43,8 @@ class TestClass : public TestFixture {
         TEST_CASE(virtualDestructorInherited);
         TEST_CASE(virtualDestructorTemplate);
 
+        TEST_CASE(virtualDestructorInconclusive); // ticket # 5807
+
         TEST_CASE(copyConstructor1);
         TEST_CASE(copyConstructor2); // ticket #4458
 
@@ -1737,11 +1739,12 @@ class TestClass : public TestFixture {
     }
 
     // Check that base classes have virtual destructors
-    void checkVirtualDestructor(const char code[]) {
+    void checkVirtualDestructor(const char code[], bool inconclusive = false) {
         // Clear the error log
         errout.str("");
 
         Settings settings;
+        settings.inconclusive = inconclusive;
 
         // Tokenize..
         Tokenizer tokenizer(&settings, this);
@@ -1991,6 +1994,30 @@ class TestClass : public TestFixture {
         ASSERT_EQUALS("[test.cpp:9]: (error) Class 'AA<double>' which is inherited by class 'B' does not have a virtual destructor.\n", errout.str());
     }
 
+    void virtualDestructorInconclusive() {
+        checkVirtualDestructor("class Base {\n"
+                               "public:\n"
+                               "    ~Base(){}\n"
+                               "    virtual void foo(){}\n"
+                               "};\n", true);
+        ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Class 'Base' which has virtual members does not have a virtual destructor.\n", errout.str());
+
+        checkVirtualDestructor("class Base {\n"
+                               "public:\n"
+                               "    ~Base(){}\n"
+                               "    virtual void foo(){}\n"
+                               "};\n"
+                               "class Derived : public Base {\n"
+                               "public:\n"
+                               "    ~Derived() { bar(); }\n"
+                               "};\n"
+                               "void foo() {\n"
+                               "    Base * base = new Derived();\n"
+                               "    delete base;\n"
+                               "}\n", true);
+        ASSERT_EQUALS("[test.cpp:3]: (error) Class 'Base' which is inherited by class 'Derived' does not have a virtual destructor.\n", errout.str());
+    }
+
     void checkNoConstructor(const char code[], const char* level="style") {
         // Clear the error log
         errout.str("");
