Improve performance of virtualDestructor check

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.

When the first base class is checked, the pointer variables of each type
are collected 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.

Author: Oipo

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;
                 }
 

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() {