How can I return an Object that has itself allocated space on the heap in c++?

Question

Solved

Solution: provide the copy constructor and follow the rule of three.

Original problem

I'm implementing a Class that is a Matrix of intervalls. My Intervall Class looks like this (shortned):

class Intervall
{
friend std::ostream& operator<<(std::ostream& output, const Intervall& i);

public:
    Intervall();
    Intervall(double n);

private:
    double min_value;
    double max_value;
};

With this implementation:

Intervall::Intervall(){
    min_value = 0;
    max_value = 0;
}

Intervall::Intervall(double n){
    min_value = n;
    max_value = n;
}

ostream& operator<<(ostream& output, const Intervall& i){
    output << "[" <<  i.min_value << ", " << i.max_value <<"]";
    return output;
}

My Matrix class looks like this:

class IntervallRMatrix
{
friend std::ostream& operator<<(std::ostream& output, const IntervallRMatrix& irm);

public:
   IntervallRMatrix();
   ~IntervallRMatrix();

   void setIdentity();

   IntervallRMatrix clone();

private:
   void initialize();

   Intervall* data;
   Intervall** cells;
};

with the following implementation:

IntervallRMatrix::IntervallRMatrix(){
    initialize();
}

IntervallRMatrix::~IntervallRMatrix(){
    delete cells;
    delete data;
}

ostream& operator<<(ostream& output, const IntervallRMatrix& irm){
    output << "3 x 3" << endl;
    for(int i=0; i<3; i++){
        for(int j=0; j<3; j++){
            output << irm.cells[i][j];
        }
        output << endl;
    }
    return output;
}

void IntervallRMatrix::setIdentity(){
    cells[0][0] = Intervall(1);
    cells[1][1] = Intervall(1);
    cells[2][2] = Intervall(1);
}

IntervallRMatrix IntervallRMatrix::clone(){
    IntervallRMatrix result;
    for(int i=0; i<3; i++){
        for(int j=0; j<3; j++){
            result.cells[i][j] = cells[i][j];
        }
    }
    return result;
}

void IntervallRMatrix::initialize(){
    data = new Intervall[9];
    for(int i=0; i<9; i++){
        data[i] = Intervall();
    }
    cells = new Intervall*[3];
    for(int i=0; i<3; i++){
        cells[i] = &data[3*i];
    }
}

This all works fine until I use the clone function. This code produces a memory error:

int main()
{
    IntervallRMatrix m1;
    m1.setIdentity();
    cout << "m1: " << m1 << endl;

    IntervallRMatrix m2 = m1.clone();
    cout << "m2: " << m2 << endl;
    return 0;
}

The first cout works as intended. The second cout doesn't. The error comes right when the program tries to read m2.cells[0][0].

This error does not occur if I don't delete cells and data in the destructor.

I declare cells and data in the way I do because I want to overload the square brackets so that I can later write Intervall = Matrix[i][j]. I also want to overload the arithmetic operators so that I can write Matrix m3 = m2*m1. In fact, I already did that, that's where the error first occured.

Now, finally my Question: How can I implement a function that returns an IntervallRMatrix that won't cause a memory error while still being able to free the allocated memory in the destructor?

Instead of (or additionally to) a `clone` method, consider implementing a copy constructor and assignment operator. — Sander De Dycker, Jan 31 '13 at 15:55

score 2 · Answer 1 · edited May 23 '17 at 12:12

2

The best solution is to avoid manual memory management. Use an appropriate container (e.g. std::vector), or a smart pointer.

If you must do manual management, then ensure you follow the Rule of Three.

edited May 23 '17 at 12:12

Community

1
1

answered Jan 31 '13 at 15:50

Oliver Charlesworth

267,707
33
569
680

Thanks! I ran into other problems before when using vectors, but providing the copy constructor as mentioned in the rule of three worked. – fholStack Jan 31 '13 at 16:43

score 0 · Accepted Answer · answered Jan 31 '13 at 15:59

The issue here is that the clone method returns a copy of the result object you just created on the stack and initialised. As you did not implment any copy constructor (from what we can see) a default one is provided by the compiler. This default implementation will basically copy the member variable from one instance to another (in this case, the pointers to cells and data). The content will not be duplicated at all. Once you get out of the clone function scope, the result object gets deleted (as pre desctructor code) and you end up with a copy of result with the cells and data pointers still pointing to the just free memory. This leads to an undefined behavior (often read as "it will crash")

Follow the Rule of Three as Oli Charlesworth put in his answer.

Thanks a million, providing the copy constructor did the trick. — fholStack, Jan 31 '13 at 16:40

How can I return an Object that has itself allocated space on the heap in c++?

2 Answers2