Fix #15143: Improve performance in the case of many template instantiations

lib/checkclass.cpp

@@ -2025,6 +2025,39 @@ void CheckClassImpl::virtualDestructor()
 
     logChecker("CheckClass::virtualDestructor");
 
+    // pointer variables of type 'Base *', for each type 'Base'
+    std::unordered_map<const Type *, std::vector<int>> pointersByType;
+
+    // for each pointer variable, the classes 'Derived' of a "ptr = new Derived" that is followed by a "delete ptr;"
+    std::unordered_map<int, std::set<std::string>> deletedAfterNew;
+
+    // collect these when the first base class is checked
+    bool deletionsCollected = false;
+    const auto collectDeletions = [&]() {
+        if (deletionsCollected)
+            return;
+        deletionsCollected = true;
+
+        for (const Variable* var : mSymbolDatabase->variableList()) {
+            if (var && var->isPointer() && var->type())
+                pointersByType[var->type()].push_back(var->declarationId());
+        }
+
+        // the classes 'Derived' of the "ptr = new Derived" seen so far
+        std::unordered_map<int, std::set<std::string>> 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)
@@ -2073,40 +2106,18 @@ 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<int> baseClassPointers;
+                    collectDeletions();
 
-                    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<int> 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;
-                        }
-                    }
+                    // pointer variables of type 'Base *'
+                    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;
                 }
 

lib/tokenize.cpp

@@ -5220,6 +5220,20 @@ static std::string getScopeName(const std::list<ScopeInfo2> &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<std::string> &scope, const Token *nsToken, Token *memberToken, const std::list<ScopeInfo2> &scopeInfo)
 {
     auto scopeIt = scopeInfo.cbegin();
@@ -5252,16 +5266,11 @@ static Token * matchMemberName(const std::list<std::string> &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<ScopeInfo2>
     return nullptr;
 }
 
+using MembersByScopeName = std::unordered_map<std::string, std::vector<const Member *>>;
+
+// 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<Member> &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<const Member *> &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<const Member *> &membersForScope(const MembersByScopeName &index, const std::list<ScopeInfo2> &scopeInfo)
+{
+    static const std::vector<const Member *> 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<ScopeInfo2> &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<ScopeInfo2> 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;
 

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 T> class Derived : public Base { public: ~Derived() { (void)11; } };\n"
+                               "void f(Base *p) {\n"
+                               "    p = new Derived<unsigned int>;\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() {

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<class T> struct C {\n"
+                            "            struct D { int x; void f(); };\n"
+                            "        };\n"
+                            "    }\n"
+                            "}\n"
+                            "using namespace A;\n"
+                            "template<class T> void B::C<T>::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"