CODEWITHSUNDEEP
c++valgrinddestructordelete-operator
NeviJ
NeviJ

Reputation: 81

Why destructor doesn't free array memory?

My question is why destructor doesn't free memory of my temp array? Valgrind tells me that I used new operator in my constructor but didn't delete memory after that. When I simply write delete temp, I got many errors in Valgrind like Invalid read of size, double free etc. Can you tell me guys what's going on here?

array_xyz(const int r, const int c, double **arg_array) {

    rows = r;
    cols = c;
    array_xyz *temp = new array_xyz();
    temp->arr = new double *[rows];
    temp->rows = r;
    temp->cols = c;
    arr = new double *[rows];

    for (int i = 0; i < rows; i++) {
        arr[i] = new double [cols];
        temp->arr[i] = new double [cols];
    }

    for (int j = 0; j < rows; j++) {
        for (int k = 0; k < cols; k++)
            temp->arr[j][k] = arg_array[j][k];
    }

    arr = temp->arr;
    //delete temp; -> doesn't work, valgrind tells that I free memory twice
}

array_xyz() {
    rows = 0;
    cols = 0;
    arr = NULL;
}

~array_xyz() {
    for (int i = 0; i < rows; i++)
        delete []arr[i];
    delete []arr;
}

Upvotes: 1

Views: 979

Answers (3)

Vlad from Moscow
Vlad from Moscow

Reputation: 310910

After this statement

arr = temp->arr;

the both pointers arr and temp->arr points to the same extend of memory.

If you add this satetment

delete temp

then the destructor of the class array_xyz frees this extent of memory (and the extents pointed to by elements of the dynamically allocated array). Also the destructor of the created object also will delete the same extent(s) of memory because its own pointer arr points to the same memory. So there will be attempts to free the same memory extents twice.

It is not clear why you are using an intermediate dynamically created object pointed to by the pointer temp. It is entirely a redundant code that only confuses readers of the constructor.

Upvotes: 2

user31264
user31264

Reputation: 6727

You allocate both arr (and all its rows) and temp_arr (and all its rows). Then you do arr=temp_arr;. It does NOT copy values of temp_arr to arr. Instead, it forces arr to point to the same address as temp_arr. The whole memory allocated to arr previously is now orphaned (there is no pointer to it, so you cannot free it, and it serves no usable purpose). If you delete temp_arr, it would automatically delete arr, as they now point to the same place in memory.

Upvotes: 5

NeviJ
NeviJ

Reputation: 81

Thank you I got it. I wanted to copy values from arg_array to arr, good point that temp wasn't even needed there.

Here's solution:

  array_xyz(const int r, const int c, double **arg_array) {

    rows = r;
    cols = c;
    arr = new double *[rows];

    for (int i = 0; i < rows; i++) {
        arr[i] = new double [cols];
    }

    for (int j = 0; j < rows; j++) {
        for (int k = 0; k < cols; k++)
            arr[j][k] = arg_array[j][k];
    }

}

Upvotes: 1

Related Questions