From ab7bb876f9d8f624a712a803232d5834a7bc4b80 Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Fri, 13 Aug 2010 18:34:02 +0200 Subject: [PATCH] Symbol database: Refactoring virtual destructors check. Ticket: #1895 --- lib/checkclass.cpp | 130 ++++++++++++--------------------------------- lib/checkclass.h | 11 ++++ test/testclass.cpp | 2 +- 3 files changed, 47 insertions(+), 96 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 35e9eee66..529fd7b1e 100644 --- a/lib/checkclass.cpp +++ b/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(); - while ((derived = Token::findmatch(derived, pattern_classdecl)) != NULL) + std::multimap::const_iterator i; + + for (i = spaceInfoMMap.begin(); i != spaceInfoMMap.end(); ++i) { - // 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()); + const SpaceInfo *info = i->second; - // No destructor.. - if (! derived_destructor) - { - derived = derived->next(); - continue; - } + // Skip base classes and namespaces + if (info->derivedFrom.empty()) + continue; - // Empty destructor.. - if (Token::Match(derived_destructor, "~ %var% ( ) { }")) - { - derived = derived->next(); - continue; - } - } + // Find the destructor + const Func *destructor = info->getDestructor(); + // 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); - while (Token::Match(derived, "%var%")) + for (unsigned int j = 0; j < info->derivedFrom.size(); ++j) { - 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 - if (Token::Match(derived, "public|protected|private")) - derived = derived->next(); + const SpaceInfo *spaceInfo = info->derivedFrom[j].spaceInfo; // Name of base class.. - const std::string baseName = derived->strAt(0); - - // 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; + const std::string baseName = spaceInfo->className; // Find the destructor declaration for the base class. - const Token *base = Token::findmatch(_tokenizer->tokens(), (std::string("%any% ~ ") + baseName + " (").c_str()); - while (base && base->str() == "::") - base = Token::findmatch(base->next(), (std::string("%any% ~ ") + baseName + " (").c_str()); - - const Token *reverseTok = base; - while (Token::Match(base, "%var%") && base->str() != "virtual") - base = base->previous(); + const Func *base_destructor = spaceInfo->getDestructor(); + const Token *base = 0; + if (base_destructor) + base = base_destructor->token->tokAt(-2); // Check that there is a destructor.. - if (! base) + if (!base_destructor) { - // Is the class declaration available? - base = Token::findmatch(_tokenizer->tokens(), (std::string("class ") + baseName + " {").c_str()); - if (base) - { - virtualDestructorError(base, baseName, derivedClass->str()); - } + if (spaceInfo->derivedFrom.empty()) + virtualDestructorError(spaceInfo->classDef, baseName, derivedClass->str()); continue; } - - // There is a destructor. Check that it's virtual.. - else if (base->str() == "virtual") + else if (base_destructor->isVirtual) 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; - 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(); - } + if (base_destructor->access == Public) + virtualDestructorError(base, baseName, derivedClass->str()); } } } diff --git a/lib/checkclass.h b/lib/checkclass.h index 0aa4ba33d..c72b134c1 100644 --- a/lib/checkclass.h +++ b/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 &callstack); + + const Func *getDestructor() const + { + std::list::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 */ diff --git a/test/testclass.cpp b/test/testclass.cpp index b97d8573f..07d24f04d 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -1418,7 +1418,7 @@ private: " public:\n" " ~B(){int a;}\n" "};\n"); - ASSERT_EQUALS("[test.cpp:7]: (error) Class AA which is inherited by class B does not have a virtual destructor\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8]: (error) Class AA which is inherited by class B does not have a virtual destructor\n", errout.str()); } void checkUninitVar(const char code[])