C ++ constructor with new

Question

I make very stupid mistakes by simply wrapping a pointer to some new 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 populate the vector with class instances, do the internal data pointers all point to the same memory?

Answer 1

1. You are missing a copy constructor.

2. The assignment operator should not just copy the pointer, because it leaves several Matrix objects with the same data pointer, which means that the pointer will delete d several times. Instead, you should create a deep copy of the matrix. See this copy and swap idiom question in which @GMan gives a detailed explanation of how to write an effective safe operator= function.

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

Answer 2

Whenever you write one of the copy constructors, copy assignment operator, or destructor, you must do all three. This is the Big Three, and the previous rule is the Rule of Three.

Right now, your copy constructor is not making a deep copy. I also recommend that you use the copy and swap idiom whenever you implement The Big Three. * In its current form, your operator= incorrect.

---

This may be a training exercise, but you should always give the lessons responsibly. Right now you have two: memory resource management and Matrix . You must separate them so that you have one class that processes the resource and another that uses the specified class to use the resource.

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

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

Answer 3

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 should use delete[] instead of delete . Also, the assignment operator simply copies the address of an already allocated array and does not make a deep copy.

Answer 4

Your missing ctor instance is already listed. When you fix this, you will still have a serious problem: the assignment operator executes 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 a new space, copy the existing content to a new space), or use something like reference counting to ensure that the data will be deleted only once, when the last link to it will be destroyed.

Edit: at the risk of editing, what you posted is mostly a poster child, because you should use a standard container instead of writing your own. If you want a rectangular matrix, consider it as a wrapper around the vector .

Answer 5

new[] , but you are not using delete[] . This is a really bad idea .

And your assignment operator makes two instances refer to the same allocated memory - both of them will try to free it! Oh, and you leak the left side memory during the assignment.

And yes, you do not have a copy constructor . This is what rule 3 says .

Answer 6

The problem is that you are creating a temporary with Matrix(100,100) , which is destroyed after it is finely copied into the vector. Then, at the next iteration, it is built 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 code to remove objects in the matrix when you are done.

EDIT: Also correct the material mentioned in other answers. This is also important. But if you change your copy constructor and assignment operators to make deep copies, do not make “new” objects when filling in a vector or memory leak.