CODEWITHSUNDEEP
c++dynamicmemory-management
B.K.
B.K.

Reputation: 10172

Deleting dynamic objects with a destructor? C++

I have a few objects throughout the program that are facing a similar issue. One example:

I have an Image class:

class Image
{
public:
    Image();
    Image(const Image& copy);
    ~Image();

    void loadimage(string filename);
    void saveimage(string filename);
    Image superimpose(const Image& ontop, Color mask);

    int getwidth();
    int getheight();
    Image operator=(const Image&);

protected:    
    Color** pixels;
    int width;
    int height;
    ImageLoader* loader;
};

It has a copy constructor:

Image::Image(const Image& copy)
{
    width = copy.width;
    height = copy.height;
    loader = copy.loader;

    pixels = new Color*[height];
    for(int i = 0; i < height; i++)
    {
        pixels[i] = new Color[width];
    }

    for(int h = 0; h < height; h++)
    {
        for(int w = 0; w < width; w++)
        {
            pixels[h][w] = copy.pixels[h][w];
        }
    }
}

Color is a struct:

struct Color
{
    unsigned int r;
    unsigned int g;
    unsigned int b;
};

My concern is that I create a dynamic two-dimensional array of Color structs, but I'm not sure when and where to delete it. I implemented the following in my Image destructor, but I'm not 100% certain that it's doing the job and I'm not sure how to check if it does:

Image::~Image()
{
    for(int i = 0; i < height; i++)
    {
        delete[] pixels[i];
    }

    delete[] pixels;
    pixels = NULL;
}

Am I implementing the memory deallocation correctly?

Upvotes: 0

Views: 2100

Answers (3)

masoud
masoud

Reputation: 56549

It's OK.

Two points, you can use unique_ptr or shared_ptr and get rid of self deleting the memory.

Second is, I prefer use nullptr or 0 instead of NULL (although it's standard). Moreover, since the container object is destroying, you don't need to set its member to null.

And the best is using std::vector:

std::vector<std::vector<Color>> pixels;

...

Image::Image(const Image& copy)
{
    width = copy.width;
    height = copy.height;
    loader = copy.loader;

    pixels.resize(height);
    for (int i = 0; i < height; i++)
    {
        pixels[i].resize(width);
    }

    for(int h = 0; h < height; h++)
    {
        for(int w = 0; w < width; w++)
        {
            pixels[h][w] = copy.pixels[h][w];
        }
    }
}

Upvotes: 3

Matthew Hoggan
Matthew Hoggan

Reputation: 7604

I see a lack of RAII here and problems with potential exceptions thrown like bad_alloc which will leave your object in an undefined state. I would allocate the memomry in an object that manages memory for you and so when the object is out of scope it will destroy itself. Said object should also provide a copy construtor that deep copies memory and allows you to access the raw data through a pointer to the first byte. std::vector has all these attributes.

Upvotes: 2

taocp
taocp

Reputation: 23664

Am I implementing the memory deallocation correctly?

Yes. It is correct. However, you'd better replace the 2D dynamic array with vector of vectors, which manages memory by itself, safer and less error prone.

Upvotes: 2

Related Questions