C++

Forum

 
Allocating and deallocating 2D arrays

wenadin (3)
Hi, I'm working on a project for a class and have to use a dynamically allocated 2D array to hold and manipulate an image. I am having issues actually doing this and whenever my program ends, I get a **glibc detected** ./driver: double free or corruption error. I have tracked the error down to my destructor and have a feeling that I'm doing it wrong, but I am horribly confused by which order I need to deallocate the memory.
Here is my constructor for the class I am using: 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
 
Image::Image(int col, int row, int grays)
{
	rows = row;
	cols = col;
	grayMax = grays;
	pixelVal = new int *[cols];
	for(int colCount = 0; colCount < cols; colCount++)
	{
		pixelVal[colCount] = new int[rows];
		for(int rowCount = 0; rowCount < rows; rowCount++)
		{
			pixelVal[colCount][rowCount] = 0;
		}
	}
}

And here is what I wrote for the destructor:
1
2
3
4
5
6
7
8
 
Image::~Image()
{
	for(int delCol = 0; delCol < cols; delCol++)
	{
		delete [] pixelVal[delCol];
	}
	delete [] pixelVal;
}

Any help with this would be greatly appreciated.
Cubbi (4774)
What are your copy constructor and copy assignment operator?
Also, check your accesses: your data structure holds cols arrays of rows elements in each. If you're ever accessing it using the more traditional [row] [col] indexing, you may be corrupting the heap then.
Also, why arrays?
wenadin (3)
Right now, the copy constructor and assignment operator are not in use by the program. The copy constructor also uses a similar algorithm:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
 
Image::Image(const Image &copyImage)
{
	cols = copyImage.cols;
	rows = copyImage.rows;
	grayMax = copyImage.grayMax;
	pixelVal = new int *[cols];
	for(int colCount = 0; colCount < cols; colCount++)
	{
		pixelVal[colCount] = new int[rows];
		for(int rowCount = 0; rowCount < rows; rowCount++)
		{
			pixelVal[colCount][rowCount] = copyImage.pixelVal[colCount][rowCount];
		}
	}
}


As far as the copy assignment operator goes, that hasn't been written yet, but if it had, it would look similar to the copy constructor. I'm trying to test functionality as I add it so that I don't get a huge group of errors when the program is complete.

Using arrays was part of the assignment. I know that when I'm introduced to a better way to do this, I will not look back because this is proving to be quite the pain.
Last edited on
Cubbi (4774)
So far I am unable to reproduce any problems. Adding a proper copy assignment, my test program was:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
 
#include <algorithm>
class Image {
    int rows;
    int cols;
    int grayMax;
    int** pixelVal;
 public:
    Image(int, int, int);
    ~Image();
    Image(const Image &);
    Image& operator=(Image);
};

Image::Image(int col, int row, int grays)
{
        rows = row;
        cols = col;
        grayMax = grays;
        pixelVal = new int *[cols];
        for(int colCount = 0; colCount < cols; colCount++)
        {
                pixelVal[colCount] = new int[rows];
                for(int rowCount = 0; rowCount < rows; rowCount++)
                {
                        pixelVal[colCount][rowCount] = 0;
                }
        }
}


Image::~Image()
{
        for(int delCol = 0; delCol < cols; delCol++)
        {
                delete [] pixelVal[delCol];
        }
        delete [] pixelVal;
} 

Image::Image(const Image &copyImage)
{
        cols = copyImage.cols;
        rows = copyImage.rows;
        grayMax = copyImage.grayMax;
        pixelVal = new int *[cols];
        for(int colCount = 0; colCount < cols; colCount++)
        {
                pixelVal[colCount] = new int[rows];
                for(int rowCount = 0; rowCount < rows; rowCount++)
                {
                        pixelVal[colCount][rowCount] = copyImage.pixelVal[colCount][rowCount];
                }
        }
}

Image& Image::operator=(Image rhs)
{
    std::swap(cols, rhs.cols);
    std::swap(rows, rhs.rows);
    std::swap(grayMax, rhs.grayMax);
    std::swap(pixelVal, rhs.pixelVal);
    return *this;
}

int main()
{
    Image i1(5, 10, 1);
    Image i2(20, 10, 1);
    Image i3(i1);
    i2 = i3 = i1;
}


for which valgrind output

==4162== Memcheck, a memory error detector
==4162== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==4162== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==4162== Command: ./test
==4162== 
==4162== 
==4162== HEAP SUMMARY:
==4162==     in use at exit: 0 bytes in 0 blocks
==4162==   total heap usage: 45 allocs, 45 frees, 1,920 bytes allocated
==4162== 
==4162== All heap blocks were freed -- no leaks are possible


Perhaps you should post a complete testcase.
Last edited on
wenadin (3)
Weird, I just completely discarded the code I was using before and rewrote the constructor and destructor and it works now... I think I just needed a fresh start or I had some problem existing elsewhere in the code. Thanks for taking a look at it.
bnolsen (20)
I'm not sure if this was part of the spec, but on a modern processer allocating an image a row at a time is noticeably slower than allocating a single buffer and computing offsets into the single buffer. IMHO it's also simpler to code up and implement. Also you should likely fix the signature on operator=(Image rhs) to operator(Image && rhs) or just get rid of this function entirely.
Last edited on
Cubbi (4774)
allocating an image a row at a time is noticeably slower than allocating a single buffer

True, which is why most implementations of matrices in C++ use one-dimensional underlying storage, e.g. a vector or a valarray. That's why I asked "why arrays?"

you should likely fix the signature on operator=(Image rhs)
No, that is the correct C++ copy assignment operator.
Topic archived. No new replies allowed.