c++ constructor with new

Question

I'm making a very dumb mistake just wrapping a pointer to some new'ed memory in a simple class.

class Matrix
{
  public:
    Matrix(int w,int h) : width(w),height(h)
    {           
        data = new unsigned char[width*height];
    }

    ~Matrix() { delete data;    }

    Matrix& Matrix::operator=(const Matrix&p)
    {  
            width = p.width;
            height = p.height;
            data= p.data;
            return *this;
    }
    int width,height;
    unsigned char *data;
}

.........
// main code
std::vector<Matrix> some_data;

for (int i=0;i<N;i++) {
   some_data.push_back(Matrix(100,100)); // all Matrix.data pointers are the same
}

When I fill the vector with instances of the class, the internal data pointers all end up pointing to the same memory ?

Answer 1

1. You're missing the copy constructor.

2. Your assignment operator should not just copy the pointer because that leaves multiple Matrix objects with the same data pointer, which means that pointer will be deleted multiple times. Instead, you should create a deep copy of the matrix. See this question about the copy-and-swap idiom in which @GMan gives a thorough explanation about how to write an efficient, exception-safe operator= function.

3. You need to use delete[] in your destructor, not delete.

Answer 2

You missed the copy constructor.

Matrix(const Matrix& other) : width(other.w),height(other.h)
{           
    data = new unsigned char[width*height];
    std::copy(other.data, other.data + width*height, data); 
}

Edit: And your destructor is wrong. You need to use delete[] instead of delete. Also you assignment operator is just copying the address of the already allocated array and isn't doing a deep copy.

Answer 3

Whenever you write one of a copy-constructor, copy-assignment operator, or destructor, you should do all three. These are The Big Three, and the previous rule is The Rule of Three.

Right now, your copy-constructor doesn't do a deep copy. I also recommend you use the copy-and-swap idiom whenever you implement The Big Three.* As it stands, your operator= is incorrect.

---

Perhaps it's a learning exercise, but you should always give classes a single responsibly. Right now, yours has two: managing a memory resource, and being a Matrix. You should separate these so that you have one class that handles the resource, and another that uses said class to use the resource.

That utility class will need to implement The Big Three, but the user class will actually need not implement any of them, because the implicitly generated ones will be handled properly thanks to the utility class.

Of course, such a class already exists as std::vector.

Answer 4

Your missing copy ctor has already been pointed out. When you fix that, you'll still have a major problem though: your assignment operator is doing a shallow copy, which will give undefined behavior (deleting the same data twice). You need either a deep copy (i.e., in your operator= allocate new space, copy existing contents to new space) or else use something like reference counting to ensure the data gets deleted only once, when the last reference to it is destroyed.

Edit: at the risk of editorializing, what you've posted is basically a poster-child for why you should use a standard container instead of writing your own. If you want a rectangular matrix, consider writing it as a wrapper around a vector.

Answer 5

You're using new[], but you aren't using delete[]. That's a really bad idea.

And your assignment operator makes two instances refer to the same allocated memory - both of which will try to deallocate it! Oh, and you're leaking the left side's old memory during assignment.

And, yes, you're missing a copy constructor, too. That's what the Rule of Three is about.

Answer 6

The problem is you are creating a temporary with Matrix(100,100) that gets destructed after it is shallow copied into the vector. Then on the next iteration it is constructed again and the same memory is allocated for the next temporary object.

To fix this:

some_data.push_back(new Matrix(100,100));

You will also have to add some code to delete the objects in the matrix when you are done.

EDIT: Also fix the stuff mentioned in the other answers. That's important, too. But if you change your copy constructor and assignment operators to perform deep copies, then don't 'new' the objects when filling the vector or it will leak memory.