0

A mate of mine told me, I've a memory leak in this code

Base
{
    public:
        vector<Foo*> fooes;
};

Derived : public Base
{
    public:

        Derived ( )
        {
            for ( int i = 0 ; i < 10 ; i++ )
            {
                    this.fooes.push_back ( new Foo() );
            }
        };

};

But he is a very busy man and he can not help me, so I ask you, where is the memory leak? And how do I fix it? As I understand it, the memory leak is that I do not delete objects, created by new Foo(), so I just can add a destructor to Base, and clear fooes vector, right?

Base
{
    public:
        vector<Foo*> fooes;
        ~Base ( )
        {
            this->fooes.clear();
        };

};

The question is:

  1. Is this a correct memory leak fix?

  2. Will the destructor of Base be called before the destructor of Derived, or not?

  3. Will the fooes vertor be deleted automatically while deleting Base or I must delete all members of the class manually?

Kolyunya
  • 5,973
  • 7
  • 46
  • 81
  • What is `Foo`? Is it polymorphic? (and unrelated: the destructor of `Base` should be virtual) – R. Martinho Fernandes Sep 03 '12 at 10:29
  • 2
    You just shouldn't use vectors of dumb pointers. Either use vectors of smart pointers or use collections specifically designed to hold pointers. (There are other horrible ways this will fail, such as copy assignment or copy construction.) – David Schwartz Sep 03 '12 at 10:30
  • 3
    If you or your mate don't have time, then you absolutely mustn't use naked pointers and `new`. Make an `std::vector>`, and get on with your busy lives without worrying about memory management. – Kerrek SB Sep 03 '12 at 10:31
  • You may find this helpful: http://stackoverflow.com/questions/12068950/c-destructors-with-vectors-pointers/12068989#12068989 – hmjd Sep 03 '12 at 10:31

3 Answers3

7

1) Is this a correct memory leak fix?

No, you have to iterate through the elements and manually delete them.

2) Will the destructor of Base be called before the destructor of Derived, or not?

No (assuming you're deleting a Derived object).

3) Will the fooes vector be deleted automatically while deleting Base or I must delete all members of the class manually?

Yes & no. The vector itself will be deleted because it is managed automatically, its members will not:

~Base ( )
{
   for ( size_t i = 0 ; i < fooes.size() ; i++ )
     delete fooes[i];
};

You should have a delete and delete[] for every new and new[] respectively.

A better alternative to all this is using smart pointers.

Luchian Grigore
  • 253,575
  • 64
  • 457
  • 625
  • can we extend vector class so that it deletes itself ? example: vector_e.del_n_clear(); – huseyin tugrul buyukisik Sep 03 '12 at 10:33
  • @tuğrulbüyükışık std containers are not meant to be extended. – Luchian Grigore Sep 03 '12 at 10:35
  • @tuğrulbüyükışık what you can do is make a free function that takes a vector as parameter and delete its items: `template clearVector(T& vect) { .... };`. – Luchian Grigore Sep 03 '12 at 10:36
  • Thanks for a reply, and how do I know, that the destructor of `Base` is called when I delete the instance of `Derived`? – Kolyunya Sep 03 '12 at 10:37
  • @Luchian Grigore doesn't vector::clear do that? http://www.cplusplus.com/reference/stl/vector/clear/ – mostruash Sep 03 '12 at 10:37
  • @Kolyunya 1. Base's destructor should be virtual (google for "virtual destructor"). 2. If you don't trust me, you can just print some debug statements to be sure. – Luchian Grigore Sep 03 '12 at 10:38
  • @mostruash the vector elements are `Foo*`, so the *pointer's destructor* is called (read between the lines), not `Foo`'s. The pointers are cleared, yes, the memory they point to, no. – Luchian Grigore Sep 03 '12 at 10:39
  • @tuğrulbüyükışık: why oh why would you need this? A `std::vector>` already has *exactly* the semantics you desire. – Kerrek SB Sep 03 '12 at 10:40
3

You need to iterate over all your pointers in the vector and delete them before clearing it. The vector will clear the pointers, but it will not clear the memory they point to unless you do it yourself or change your vector to use something like a shared_ptr. If your vector were a vector of objects, then you would not have a memory leak, but since the elements are pointers that have been allocated via new you will need to release that memory - the vector will not automatically do it for you.

mathematician1975
  • 21,161
  • 6
  • 59
  • 101
  • vector::clear All the elements of the vector are dropped: their destructors are called, and then they are removed from the vector container, leaving the container with a size of 0. – Kolyunya Sep 03 '12 at 10:31
  • 3
    @Kolyunya the destructors of dynamically-allocated objects are not automatically called. – Luchian Grigore Sep 03 '12 at 10:32
  • 1
    @Kolyunya yes it will destroy the pointers, but this wont release the memory they point to - this must be done manually when not using smart pointers – mathematician1975 Sep 03 '12 at 10:33
  • 1
    @Kolyunya: The "destructor" of a naked pointer does nothing more than the destructor of an `int` - namely *nothing*. – Kerrek SB Sep 03 '12 at 10:34
1

There is no memory leak in the code example, just a class definition. If a program creates an object of type Derived and destroys it, there will probably (see below) be a memory leak. But if a program creates an object of type Derived, destroys all the Foo objects that it holds, then deletes the original object, there will probably not be a memory leak.

Memory leaks can only rarely be analyzed in isolation; they are properties of a whole program. This class could be modified to delete the Foo objects, but that could impose a new design constraint that wasn't intended. For example, Foo objects might register themselves in an internal container, with a requirement that all such objects persist until the end of the program; given that requirement, deleting the objects in the destructor of Derived would be wrong.

Now, that's all largely hypothetical; I wouldn't complain about someone enhancing the destructor of Derived to delete all the Foo objects -- it's probably the right thing to do. But there's always a bit of a doubt...

Pete Becker
  • 74,985
  • 8
  • 76
  • 165