From 55a677113009e827ef4926ec93f4aad8d0f8a000 Mon Sep 17 00:00:00 2001 From: Michael de Lang Date: Sun, 25 Oct 2026 21:12:32 +0100 Subject: [PATCH 1/2] Fix #15143 Improve performance of virtualDestructor check https://trac.cppcheck.net/ticket/15143 CheckClassImpl::virtualDestructor() scanned the whole variable list and the whole token list for every (derived class, public base class) pair. With many template instantiations (for example etl) this took up to 26% of the analysis time. The named pointer variables of each type are now collected once, and a single pass over the tokens records, for each pointer variable, the classes 'Derived' of a "ptr = new Derived" that is followed by a "delete ptr;". Checking a pair is then a lookup. This also fixes a false negative: the name of a template instantiation such as 'Derived < int >' is a single token that contains spaces, so the old pattern "new Derived < int >" never matched it and the pair was never reported. On a project using etl, glaze and Catch2 (211 translation units in two configurations) total CPU time drops from 88.7 to 61.6 minutes, with identical output for every translation unit. --- lib/checkclass.cpp | 65 ++++++++++++++++++++++++---------------------- test/testclass.cpp | 30 +++++++++++++++++++++ 2 files changed, 64 insertions(+), 31 deletions(-) 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/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() { From ab38a743b573200e0b8ef1b9e168cffe6195b0ec Mon Sep 17 00:00:00 2001 From: Michael de Lang Date: Sun, 25 Oct 2026 21:12:32 +0100 Subject: [PATCH 2/2] Fix #15143 Improve performance of setVarIdPass2 https://trac.cppcheck.net/ticket/15143 Tokenizer::setVarIdPass2() tried every out-of-class member definition for every class. matchMemberName() can only succeed when the innermost scope name of the class matches a name in the member's scope, in one of its "using namespace" or in its qualification. The members are now indexed by those names, in their original order, so each class only tries its candidates. The index and matchMemberName() walk the qualification with the same skipQualification(). On the same project total CPU time drops further from 61.6 to 52.9 minutes, with identical output for every translation unit. --- lib/tokenize.cpp | 77 ++++++++++++++++++++++++++++++++++++++-------- test/testvarid.cpp | 44 ++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 12 deletions(-) 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/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"