C++ memory leak with class members

Question

Debug_VLD in VS2010 reveals some memory leaks that come from class member creation / initialization / deletion.

my_member is a data member with type double*. In constructor, I have

my_member = NULL ;

Then in some method, i need to allocate memory for my_member. I cannot do so in constructor, since i dont know size of array yet, and/or size may different for different calls of the method. What i do in this method is checking if member is NULL. if so, i allocate space for it, if not, i can operate on array (changing value for its element with accesor []). It looks like

void MyClass::my_method()
{
    if( my_member == NULL )
        my_member = new double[n_dim] ;

    for(int k = 0 ; k < n_dim ; k++ )
         my_member[k] = k ;
}

and memory leak occurs at line my_member = new double[n_dim] ;.

In destructor, i have

delete[] my_member ;

what is wrong? how to do allocation properly ?

thanks!

do you have a copy constructor and copy assignment operator? — Mooing Duck, Oct 22 '12 at 19:01
@MooingDuck are you speaking of MyClass? why is it needed ? could you elaborate? thanks ! ! — kiriloff, Oct 22 '12 at 19:14
http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three — Mooing Duck, Oct 22 '12 at 19:15

Mr.C64 · Accepted Answer · 2012-10-22T19:27:07.773

Using std::vector<double> is the preferred way, but if you want to have raw double* and write copy constructor, move constructor and operator= "by hand", you should do something like this:

#include <assert.h>  // for assert

#include <algorithm> // for std::swap

class MyClass
{
  //
  // Raw array
  //
  double * m_ptr; // raw pointer
  size_t m_dim;   // number of items in the array

public:

  // Default constructor - creates empty vector
  MyClass()
    : m_ptr(nullptr)
    , m_dim(0)
  {
  }


  // Copy constructor
  MyClass(const MyClass& src)
    : m_ptr(nullptr)
    , m_dim(0)
  {
    // Special case of empty source
    if (src.m_dim == 0)
    {
      assert(src.m_ptr == nullptr);
      return;
    }

    // Allocate and deep-copy from source
    m_ptr = new double[src.m_dim];
    m_dim = src.m_dim;
    for (size_t i = 0; i < m_dim; i++)
      m_ptr[i] = src.m_ptr[i];
  }

  // Move constructor: steal the "guts" from src
  MyClass(MyClass&& src)
  {
    m_ptr = src.m_ptr;
    src.m_ptr = nullptr;

    m_dim = src.m_dim;
    src.m_dim = 0;
  }

  // Destructor
  ~MyClass()
  {
    delete [] m_ptr;
  }

  // Unified operator=
  MyClass& operator=(MyClass src)
  {
    std::swap(m_ptr, src.m_ptr);
    std::swap(m_dim, src.m_dim);
    return *this;
  }
};

Luchian Grigore · Answer 2 · 2012-10-23T11:25:12.253

2

If there's any other place in the code where you set my_member to NULL without calling delete[], then yes. If you don't obey the rule of three (properly implemented copy constructor and assignment operator), you'll run into all sorts of trouble.

To prevent this, use a std::vector<double> instead, where you can do:

void MyClass::my_method()
{
    my_member.resize(n_dim); // yay, learned something new here 
    for(int k = 0 ; k < n_dim ; k++ )
         my_member[k] = k ;
}

That way, you're not managing memory, so there's no need for a destructor (unless it's virtual, in which case it can be empty).

edited Oct 23 '12 at 11:25

answered Oct 22 '12 at 19:01

Luchian Grigore

253,575
64
457
625

thanks! what if i dont want to use std::vector, but only double* ? what for copy constructor / assignment operator should i implement ? – kiriloff Oct 22 '12 at 19:08
@madptr: I've added a post on managing raw `double*`, but I do think `std::vector` is the best way (much easier, less error-prone, less boiler-plate code). – Mr.C64 Oct 22 '12 at 19:24
@madptr: Why don't you want to use `std::vector`? If you compile optimized it's just as fast as a raw pointer, and all the memory management problems are done by `std::vector`. Your code becomes much simpler. (Aside: If you aren't allowed to compile with optimization on it's a different story with regard to speed of `std::vector`.) – David Hammen Oct 22 '12 at 19:36
1

I think the code should be corrected using `std::vector::resize` method, not `reserve`. – Mr.C64 Oct 22 '12 at 22:21
@Mr.C64 why is `resize` better? – Luchian Grigore Oct 23 '12 at 09:00
@LuchianGrigore: with `resize()` you change `vector` **size**, instead with `reserve()` you change vector _capacity_. If `my_member` `vector` was initially empty (e.g. default constructed), or had a size less than `n_dim`, you are writing to elements out of valid boundaries. `vector v; v.reserve(10); v[5] = 55; /* crash - v.size() is actually 0, the vector is empty. */ `. – Mr.C64 Oct 23 '12 at 09:54
@LuchianGrigore: I think you are wrong. But I opened a new question [here](http://stackoverflow.com/questions/13029299/stdvectorresize-vs-stdvectorreserve). I think it's better to discuss there than here in the comments. – Mr.C64 Oct 23 '12 at 11:20
@LuchianGrigore: how can this possibly be valid? `reserve` just ensures that future resizes can be done without reallocating. But you're still only allowed to access elements with indices between 0 and `vector.size()` – jalf Oct 23 '12 at 11:21
1

Of course your post is incorrect, you need to use push_back() on each element after reserve() or use resize() if you want to write to them directly. – CashCow Oct 23 '12 at 11:25
@CashCow not any more. But yes, I was wrong, I've always used push_back after a reserve but in the back of my head I must have thought that the elements are there. Oh well... :) – Luchian Grigore Oct 23 '12 at 11:26
I knew you were coming back to fix which is why I withheld a -1 – CashCow Oct 23 '12 at 11:32
Is this just for C++11 or a specific std implementation? It looks like the code with reserve and access with [] works fine? https://godbolt.org/z/MhgFdZ – Steve Bronder Mar 23 '20 at 03:15

C++ memory leak with class members

2 Answers2

Linked