CODEWITHSUNDEEP
c++destructor
Sergey Ivanov
Sergey Ivanov

Reputation: 3929

Destructor causes corruption of the heap.

I'm playing around with destructors and I don't understand why I get an error for this code when the main function terminates. 

#include <iostream>
#include <math.h>
#include <string>
#include <cstdint>
#include <cassert>
using namespace std;

class RGBA {
    uint8_t _red, _blue, _green, _alpha;
    int *_arr;
    int _length;
public:
    RGBA(int *arr, int length, uint8_t r = 0, uint8_t b = 0, uint8_t g = 0, uint8_t a = 255): 
      _red (r), _blue (b), _green (g), _alpha (a) {
      _arr = arr;
      _length = length;
      }
      ~RGBA() {
          cout << "Destroying object" << endl;
          delete[] _arr;
      }

    void print() {
        for (int i = 0; i < _length; ++i) {
            cout << _arr[i] << endl;
        }
        cout << static_cast<int>(_red) << " " << static_cast<int>(_blue) << " " << static_cast<int>(_green) << " " << static_cast<int>(_alpha) << endl;
    }
};


int main() {
    int arr[3] = {1,2,3};
    RGBA rgba(arr, 3);
    rgba.print();

    cin.get();
    return 0;
}

It outputs, but then when I press Enter, it prints 'Destroying object' with the following error "This may be due to a corruption of the heap, which indicates a bug in testcpp.exe or any of the DLLs it has loaded.". 

1
2
3
0 0 0 255

I use VS2010 on Win7.

Upvotes: 1

Views: 1015

Answers (5)

Guillaume Racicot
Guillaume Racicot

Reputation: 41780

That's because you aren't supposed to delete the array.

The implementation of your destructor is correct, but the array is not allocated with new[], so you must not de-allocating with delete[].

If you'd replace your array in your main with it's modernized cousin, std::array, and the pointer in your class by std::vector, you would see that assigning a array to a vector would require an allocation on the heap and a copy of each element in your array.

A quick way to fix the code would be to allocate the array using new[]:

int* arr = new int[3]{1, 2, 3};
RGBA rgba(arr, 3);
rgba.print();

Now that the array is allocated using new[], it can be safely deleted using delete[].

However, please note that in most modern code, you either use std::array<T, N>, std::vector<T> or std::unique_ptr<T[]> to manage arrays.

The resulting code that use arrays and vectors would be like this:

#include <iostream>
#include <vector>
#include <array>

struct RGBA {
    std::uint8_t red = 0, green = 0, blue = 0, alpha = 255;
    std::vector<int> arr;

    RGBA(std::vector<int> _arr) : arr{std::move(_arr)} {}

    // No destructor needed. vector manages it's own resources.

    void print() {
        for (auto&& number : arr) {
            std::cout << number << std::endl;
        }

        std::cout << static_cast<int>(red) << " "
                  << static_cast<int>(blue) << " "
                  << static_cast<int>(green) << " "
                  << static_cast<int>(alpha) << std::endl; 
    }
};

int main() {
    // Create a constant array on the stack the modern way
    constexpr std::array<int, 3> arr{1, 2, 3};

    // Copy array elements in the vector
    RGBA rgba{std::vector<int>{arr.begin(), arr.end()}};

    rgba.print();

    cin.get();
}

Upvotes: 0

Mikel F
Mikel F

Reputation: 3671

The array you are passing, arr is being allocated on the stack in your main function. You then pass the pointer to your RGBA instance which then deletes the array in its destructor. As it was not dynamically allocated in the first place, this is a bad thing.

Deleting the array in the destructor indicates that you mean to transfer ownership of the array to that class. To do that, you need to either pass a dynamically allocated array or allocate a new array in the constructor and copy the contents of the one passed by parameter.

If you do not need ownership, simply remove the delete call in the destructor.

Upvotes: 1

Mohammad Tayyab
Mohammad Tayyab

Reputation: 696

~RGBA() {
    cout << "Destroying object" << endl;
    delete[] _arr;
}

Here was your problem because delete didn't work on static array, It always work for dynamic array. delete only work for new

int *arr = new int[3];
arr[0] = 1;
arr[1] = 2;
arr[2] = 3;

this will work perfectly.

Upvotes: 1

crashmstr
crashmstr

Reputation: 28573

In your case, this is effectively what is happening:

int arr[3] = {1,2,3};
delete[] arr;

Your arr in main is in automatic storage. You pass it into your object, which assumes ownership and assumes the memory was dynamically allocated with new.

You should only delete [] what you new [].

Upvotes: 4

flogram_dev
flogram_dev

Reputation: 42858

The automatic storage duration variable int arr[3] will be automatically deallocated when the enclosing function exits.

Trying to delete[] it causes undefined behavior. Only objects allocated with new[] can be deallocated with delete[].

Upvotes: 6

Related Questions