diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 6f5bcd3a69e..3b2f335f04f 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2025,6 +2025,33 @@ void CheckClassImpl::virtualDestructor() logChecker("CheckClass::virtualDestructor"); + // pointer variables of type 'Base *', for each type 'Base' + std::unordered_map> pointersByType; + + // for each pointer variable, the classes 'Derived' of a "ptr = new Derived" that is followed by a "delete ptr;" + std::unordered_map> deletedAfterNew; + + for (const Variable* var : mSymbolDatabase->variableList()) { + if (var && var->isPointer() && var->type() && var->nameToken()) + pointersByType[var->type()].push_back(var->declarationId()); + } + + { + // the classes 'Derived' of the "ptr = new Derived" seen so far + std::unordered_map> newClasses; + + for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) { + if (Token::Match(tok, "[;{}] %var% = new %name%")) + newClasses[tok->next()->varId()].insert(tok->strAt(4)); + + else if (Token::Match(tok, "delete %var% ;")) { + const auto it = newClasses.find(tok->next()->varId()); + if (it != newClasses.end()) + deletedAfterNew[it->first].insert(it->second.cbegin(), it->second.cend()); + } + } + } + for (const Scope * scope : mSymbolDatabase->classAndStructScopes) { // Skip base classes (unless inconclusive) @@ -2074,39 +2101,15 @@ void CheckClassImpl::virtualDestructor() // If this pattern is not seen then bailout the checking of these base/derived classes { // pointer variables of type 'Base *' - std::set baseClassPointers; - - for (const Variable* var : mSymbolDatabase->variableList()) { - if (var && var->isPointer() && var->type() == derivedFrom) - baseClassPointers.insert(var->declarationId()); - } - - // pointer variables of type 'Base *' that should not be deleted - std::set dontDelete; - - // No deletion of derived class instance through base class pointer found => the code is ok - bool ok = true; - - for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) { - if (Token::Match(tok, "[;{}] %var% =") && - baseClassPointers.find(tok->next()->varId()) != baseClassPointers.end()) { - // new derived class.. - const std::string tmp("new " + derivedClass->str()); - if (Token::simpleMatch(tok->tokAt(3), tmp.c_str(), tmp.size())) { - dontDelete.insert(tok->next()->varId()); - } - } - - // Delete base class pointer that might point at derived class - else if (Token::Match(tok, "delete %var% ;") && - dontDelete.find(tok->next()->varId()) != dontDelete.end()) { - ok = false; - break; - } - } + const auto pointers = pointersByType.find(derivedFrom); + if (pointers == pointersByType.end()) + continue; // No base class pointer that points at a derived class is deleted - if (ok) + if (std::none_of(pointers->second.cbegin(), pointers->second.cend(), [&](int varId) { + const auto deleted = deletedAfterNew.find(varId); + return deleted != deletedAfterNew.end() && deleted->second.count(derivedClass->str()) > 0; + })) continue; } diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 1d5b7436405..3e28981ebce 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -5220,6 +5220,20 @@ static std::string getScopeName(const std::list &scopeInfo) return ret; } +// If tok is the name in a qualification "name ::" or "name < ... > ::", return the token after the "::" +static Token * skipQualification(Token *tok) +{ + if (!Token::Match(tok, "%name% ::|<")) + return nullptr; + if (tok->strAt(1) == "<") { + tok = tok->next()->findClosingBracket(); + if (!Token::simpleMatch(tok, "> ::")) + return nullptr; + } + return tok->tokAt(2); +} + +// indexMembersByScopeName() relies on how this matches the scope names: keep the two in sync static Token * matchMemberName(const std::list &scope, const Token *nsToken, Token *memberToken, const std::list &scopeInfo) { auto scopeIt = scopeInfo.cbegin(); @@ -5252,16 +5266,11 @@ static Token * matchMemberName(const std::list &scope, const Token // Parse member tokens.. while (scopeIt != scopeInfo.end()) { - if (!Token::Match(memberToken, "%name% ::|<")) + if (!Token::Match(memberToken, "%name%") || memberToken->str() != scopeIt->name) return nullptr; - if (memberToken->str() != scopeIt->name) + memberToken = skipQualification(memberToken); + if (!memberToken) return nullptr; - if (memberToken->strAt(1) == "<") { - memberToken = memberToken->next()->findClosingBracket(); - if (!Token::simpleMatch(memberToken, "> ::")) - return nullptr; - } - memberToken = memberToken->tokAt(2); ++scopeIt; } @@ -5288,6 +5297,47 @@ static Token * matchMemberName(const Member &member, const std::list return nullptr; } +using MembersByScopeName = std::unordered_map>; + +// matchMemberName() can only match when the last scope name is matched by a name in the +// member's scope, in one of its "using namespace" or in its qualification. Index the members +// by these names, keeping their order, so that each scope only tries its candidates. +static MembersByScopeName indexMembersByScopeName(const std::list &members) +{ + MembersByScopeName index; + for (const Member &member : members) { + // members are visited in order, so a member that is already a candidate is the last one + const auto addCandidate = [&](const std::string &name) { + std::vector &candidates = index[name]; + if (candidates.empty() || candidates.back() != &member) + candidates.push_back(&member); + }; + for (const std::string &name : member.scope) + addCandidate(name); + for (const Token *ns : member.usingnamespaces) { + for (; Token::Match(ns, "%name% ::"); ns = ns->tokAt(2)) + addCandidate(ns->str()); + if (Token::Match(ns, "%name%")) + addCandidate(ns->str()); + } + Token *tok = member.tok; + while (Token *next = skipQualification(tok)) { + addCandidate(tok->str()); + tok = next; + } + } + return index; +} + +static const std::vector &membersForScope(const MembersByScopeName &index, const std::list &scopeInfo) +{ + static const std::vector none; + if (scopeInfo.empty()) + return none; + const auto it = index.find(scopeInfo.back().name); + return it == index.end() ? none : it->second; +} + static Token * matchMemberVarName(const Member &var, const std::list &scopeInfo) { Token *tok = matchMemberName(var, scopeInfo); @@ -5399,6 +5449,9 @@ void Tokenizer::setVarIdPass2() } } + const MembersByScopeName memberFunctionsByScopeName = indexMembersByScopeName(allMemberFunctions); + const MembersByScopeName memberVarsByScopeName = indexMembersByScopeName(allMemberVars); + std::list scopeInfo; // class members.. @@ -5515,8 +5568,8 @@ void Tokenizer::setVarIdPass2() continue; // Member variables - for (const Member &var : allMemberVars) { - Token *tok2 = matchMemberVarName(var, scopeInfo); + for (const Member *var : membersForScope(memberVarsByScopeName, scopeInfo)) { + Token *tok2 = matchMemberVarName(*var, scopeInfo); if (!tok2) continue; if (tok2->varId() == 0) @@ -5527,8 +5580,8 @@ void Tokenizer::setVarIdPass2() continue; // Set variable ids in member functions for this class.. - for (const Member &func : allMemberFunctions) { - Token *tok2 = matchMemberFunctionName(func, scopeInfo); + for (const Member *func : membersForScope(memberFunctionsByScopeName, scopeInfo)) { + Token *tok2 = matchMemberFunctionName(*func, scopeInfo); if (!tok2) continue; diff --git a/test/testclass.cpp b/test/testclass.cpp index 48fd513b494..40dca5dbeb7 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -2839,6 +2839,36 @@ class TestClass : public TestFixture { checkVirtualDestructor("class Base { public: ~Base(); };\n" "class Derived : public Base { public: ~Derived() { (void)11; } };\n"); ASSERT_EQUALS("", errout_str()); + + // the pointer is deleted before it points at the derived class, or another pointer is deleted + checkVirtualDestructor("class Base { public: ~Base(); };\n" + "class Derived : public Base { public: ~Derived() { (void)11; } };\n" + "void f(Base *p, Base *q) {\n" + " delete p;\n" + " p = new Derived;\n" + " delete q;\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // only the derived class that the deleted pointer points at + checkVirtualDestructor("class Base { public: ~Base(); };\n" + "class Derived1 : public Base { public: ~Derived1() { (void)11; } };\n" + "class Derived2 : public Base { public: ~Derived2() { (void)11; } };\n" + "void f(Base *p, Base *q) {\n" + " p = new Derived1;\n" + " q = new Derived2;\n" + " delete q;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:1:23]: (error) Class 'Base' which is inherited by class 'Derived2' does not have a virtual destructor. [virtualDestructor]\n", errout_str()); + + // the derived class is a template instantiation, its name contains spaces + checkVirtualDestructor("class Base { public: ~Base(); };\n" + "template class Derived : public Base { public: ~Derived() { (void)11; } };\n" + "void f(Base *p) {\n" + " p = new Derived;\n" + " delete p;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:1:23]: (error) Class 'Base' which is inherited by class 'Derived < unsigned int >' does not have a virtual destructor. [virtualDestructor]\n", errout_str()); } void virtualDestructorProtected() { diff --git a/test/testvarid.cpp b/test/testvarid.cpp index a1874d3c5f2..b65d35db489 100644 --- a/test/testvarid.cpp +++ b/test/testvarid.cpp @@ -240,6 +240,8 @@ class TestVarID : public TestFixture { TEST_CASE(usingNamespace1); TEST_CASE(usingNamespace2); TEST_CASE(usingNamespace3); + TEST_CASE(usingNamespace4); + TEST_CASE(usingNamespace5); TEST_CASE(setVarIdStructMembers1); @@ -4410,6 +4412,48 @@ class TestVarID : public TestFixture { ASSERT_EQUALS(expected, tokenize(code)); } + void usingNamespace4() { + // the innermost scope name is matched by the qualification of the member, after a template argument list + const char code[] = "namespace A {\n" + " namespace B {\n" + " template struct C {\n" + " struct D { int x; void f(); };\n" + " };\n" + " }\n" + "}\n" + "using namespace A;\n" + "template void B::C::D::f() { x = 0; }\n"; + + const char expected[] = "1: namespace A {\n" + "2: namespace B {\n" + "3: template < class T > struct C {\n" + "4: struct D { int x@1 ; void f ( ) ; } ;\n" + "5: } ;\n" + "6: }\n" + "7: }\n" + "8: using namespace A ;\n" + "9: template < class T > void B :: C < T > :: D :: f ( ) { x@1 = 0 ; }\n"; + + ASSERT_EQUALS(expected, tokenize(code)); + } + + void usingNamespace5() { + // the innermost scope name is matched by the "using namespace" + const char code[] = "namespace A {\n" + " namespace B { int x; }\n" + "}\n" + "using namespace A::B;\n" + "void f() { x = 0; }\n"; + + const char expected[] = "1: namespace A {\n" + "2: namespace B { int x@1 ; }\n" + "3: }\n" + "4: using namespace A :: B ;\n" + "5: void f ( ) { x@1 = 0 ; }\n"; + + ASSERT_EQUALS(expected, tokenize(code)); + } + void setVarIdStructMembers1() { const char code[] = "void f(Foo foo)\n" "{\n"