Deleting templated class pointers in a non-templated class destructor?

Question

The below code test-templated-destructor.cpp replicates the organization of a library I'm using. I'm using:

$ cat /etc/issue
Ubuntu 14.04.5 LTS \n \l
$ g++ --version
g++ (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4
$ g++ -std=c++14
g++: error: unrecognized command line option ‘-std=c++14’
g++: fatal error: no input files
compilation terminated.
$ g++ -std=c++11
g++: fatal error: no input files
compilation terminated.

There is:

• Base class AA, and classes derived from it BB and CC;
• Abstract class AAInstancer, and class derived from it AAInstancerTemplated which is templated
• class AAHandler, which has a templated function addTemplatedObject, which stores AAInstancer* pointers to new AAInstancerTemplated<T>() objects, in a map property of the class
• in main(), an AAHandler object is instantiated, and .addTemplatedObject<BB>("BB"); called on it

If I run valgrind on this, it reports:

==21000== 43 (16 direct, 27 indirect) bytes in 1 blocks are definitely lost in loss record 2 of 2
==21000==    at 0x4C2B0E0: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21000==    by 0x40141B: void AAHandler::addTemplatedObject<BB>(std::string) (test-templated-destructor.cpp:64)
==21000==    by 0x40113E: main (test-templated-destructor.cpp:82)

I think the problem is that we used new in addTemplatedObject(), thus we should correspondingly delete it latest at program exit - but that is not done, and thus the reason for the leak.

So I thought, to write an iterator that loops through the instancers map, and deletes these pointers in the desctructor of the AAHandler, but I can't:

• If I write:

  ~AAHandler() {
    cout << "  (running AAHandler destructor)" << endl;
    map<string, AAInstancer*>::iterator it;
    for ( it = instancers.begin(); it != instancers.end(); it++ ) {
      delete it->second;
    }
  }

... then I get at compilation:

$ g++ -g -Wall test-templated-destructor.cpp -o test-templated-destructor.exe
test-templated-destructor.cpp: In destructor ‘AAHandler::~AAHandler()’:
test-templated-destructor.cpp:60:18: warning: deleting object of abstract class type ‘AAInstancer’ which has non-virtual destructor will cause undefined behaviour [-Wdelete-non-virtual-dtor]
       delete it->second;
                  ^

... and it sounds right - AAInstancer doesn't have a destructor defined, so compiler probably auto-added is as non-virtual, leading to this warning (although, running this through valgrind will show that leaks are not detected anymore).

• If I write:

  template <class T>
  ~AAHandler() {
      cout << "  (running AAHandler destructor)" << endl;
      map<string, AAInstancer*>::iterator it;
      for ( it = instancers.begin(); it != instancers.end(); it++ ) {
        delete (AAInstancerTemplated<T>*)it->second;
      }
    }

... in hope that this destructor would get called if we called addTemplatedObject with some template (which it wouldn't anyway), compilation fails:

$ g++ -g -Wall test-templated-destructor.cpp -o test-templated-destructor.exe && ./test-templated-destructor.exe
test-templated-destructor.cpp:57:14: error: destructor ‘AAHandler::~AAHandler()’ declared as member template
   ~AAHandler() {
              ^

... and this makes sense too: AAHandler is a non-templated class, so probably its destructor shouldn't be templated either.

So, is it possible to write a destructor for AAHandler, which would delete all new pointers in its instancers, regardless of which template they were instantiated with - with minimal (or best, no) changes to the already existing code?

test-templated-destructor.cpp

// g++ -g -Wall test-templated-destructor.cpp -o test-templated-destructor.exe && ./test-templated-destructor.exe
// valgrind --leak-check=yes ./test-templated-destructor.exe

#include <iostream>
#include <map>
using namespace std;

class AA {
public:
  string myname;
  AA() {
    myname = "";
    cout << "  AA instantiated\n";
  }
};


class BB : public AA {
public:
  string mystuff;
  BB() {
    mystuff = "";
    cout << "  BB instantiated\n";
  }
};

class CC : public AA {
public:
  string mythings;
  CC() {
    mythings = "";
    cout << "  CC instantiated\n";
  }
};

class AAInstancer
{
public:
    virtual AA* createInstance() = 0;
    string tagName;
};

template <class T>
class AAInstancerTemplated: public AAInstancer
{
public:
    AA* createInstance() {
        return new T();
    }
};


class AAHandler
{
public:
    ~AAHandler() { }
    AAHandler() { }
    static map<string, AAInstancer*> instancers;

    template <class T>
    static void addTemplatedObject(string tagName) {
        AAInstancer* instancer = new AAInstancerTemplated<T>();
        instancer->tagName = tagName;
        instancers[tagName] = instancer;
    }

  AAHandler* get() {
    if(singleton == NULL)
      singleton = new AAHandler();
    return singleton;
  }
private:
    static AAHandler* singleton;
};
map<string, AAInstancer*> AAHandler::instancers;



int main()
{
  AAHandler aah;
  aah.addTemplatedObject<BB>("BB");

  cout << "Address of aah: " << static_cast<void*>(&aah) << endl;
  return 0;
}

Answer 1

OK, finally got something to work which compiles fine under just c++11 and does not leak; valgrind reports:

$ valgrind --leak-check=yes ./test-templated-destructor.exe==22888== Memcheck, a memory error detector
==22888== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==22888== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==22888== Command: ./test-templated-destructor.exe
==22888== 
Address of aah: 0xffefffb3f
  (running AAHandler destructor)
  ~AAInstancerTemplated <2BB> here; tref: 0x5a200b0
    ~AAInstancer here
  ~AAInstancerTemplated <2CC> here; tref: 0x5a201e0
    ~AAInstancer here
==22888== 
==22888== HEAP SUMMARY:
==22888==     in use at exit: 0 bytes in 0 blocks
==22888==   total heap usage: 6 allocs, 6 frees, 198 bytes allocated
==22888== 
==22888== All heap blocks were freed -- no leaks are possible
==22888== 
==22888== For counts of detected and suppressed errors, rerun with: -v
==22888== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Yup, no leaks - I like that :)

The approach is a bit classic: make AAInstancerTemplated keep a reference (here tref) to whatever it is instantiating via new, then create a destructor for it (for AAInstancerTemplated) that deletes this reference.

Note that even if in AAHandler, we store generic pointers AAInstancer* in instancers, while we instantiate templated objects (new AAInstancerTemplated<T>();) -- now, with this organisation, when we delete it->second which is of type AAInstancer*, the right templated destructor gets called.

The fixed test-templated-destructor.cpp:

// g++ -g -std=c++11 test-templated-destructor.cpp -o test-templated-destructor.exe && ./test-templated-destructor.exe
// valgrind --leak-check=yes ./test-templated-destructor.exe

#include <iostream>
#include <map>
#include <typeinfo>
#include <functional> // note: uses std::function, which is c++11 feature

using namespace std;

class AA {
public:
  string myname;
  AA() {
    myname = "";
    cout << "  AA instantiated\n";
  }
};


class BB : public AA {
public:
  string mystuff;
  BB() {
    mystuff = "";
    cout << "  BB instantiated\n";
  }
};

class CC : public AA {
public:
  string mythings;
  CC() {
    mythings = "";
    cout << "  CC instantiated\n";
  }
};

class AAInstancer
{
public:
  virtual ~AAInstancer() {
    cout << "    ~AAInstancer here" << endl;
  }
  virtual AA* createInstance() = 0;
  string tagName;
};

template <class T>
class AAInstancerTemplated: public AAInstancer
{
public:
  T* tref;
  AA* createInstance() {
    if (tref) delete tref;
    tref = new T();
    return tref;
  }
  ~AAInstancerTemplated() {
    cout << "  ~AAInstancerTemplated <" << typeid(T).name() << "> here; tref: " << static_cast<void*>(&tref) << endl;
    if (tref) delete tref;
  }
};


class AAHandler
{
public:
  ~AAHandler() {
    cout << "  (running AAHandler destructor)" << endl;
    typedef typename map<string, AAInstancer*>::iterator instIterator;
    for ( instIterator it = instancers.begin(); it != instancers.end(); it++ ) {
      delete it->second;
    }
  }
  AAHandler() { }
  static map<string, AAInstancer*> instancers;

  template <class T>
  static void addTemplatedObject(string tagName) {
    AAInstancer* instancer = new AAInstancerTemplated<T>();
    instancer->tagName = tagName;
    instancers[tagName] = instancer;
  }

  AAHandler* get() {
    if(singleton == NULL)
      singleton = new AAHandler();
    return singleton;
  }
private:
  static AAHandler* singleton;
};
map<string, AAInstancer*> AAHandler::instancers;

int main()
{
  AAHandler aah;
  aah.addTemplatedObject<BB>("BB");
  aah.addTemplatedObject<CC>("CC");

  cout << "Address of aah: " << static_cast<void*>(&aah) << endl;
  return 0;
}

Answer 2

Use a std::unique_ptr<AAInstancer>:

map<string, std::unique_ptr<AAInstancer>>

as member, instead of managing the memory yourself.

Answer 3

AAInstancer needs a virtual destructor. If it doesn't need a body you can default it.

virtual ~AAInstancer() = default;