diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 288afe711..05a0b2d7b 100644 --- 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 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::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::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::iterator found = find(inconclusive_errors.begin(), inconclusive_errors.end(), base_destructor); + if (found != inconclusive_errors.end()) + inconclusive_errors.erase(found); + } } } } } } + + for (std::list::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 index a2ac1ef60..8540ce1a1 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -142,7 +142,7 @@ private: 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 @@ private: 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 index a51d9ed04..37e3f5fc4 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -43,6 +43,8 @@ private: TEST_CASE(virtualDestructorInherited); TEST_CASE(virtualDestructorTemplate); + TEST_CASE(virtualDestructorInconclusive); // ticket # 5807 + TEST_CASE(copyConstructor1); TEST_CASE(copyConstructor2); // ticket #4458 @@ -1737,11 +1739,12 @@ private: } // 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 @@ private: ASSERT_EQUALS("[test.cpp:9]: (error) Class 'AA' 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("");