Creating proper copy constructor for a dynamic 2d array

Question

My program is seg faulting when the copy constructor is invoked. This is what my constructor looks like for my Grid class:

Grid::Grid(unsigned int grid_size) {
    size = grid_size;
    grid = new char *[size];
    for(int i = 0; i < size; i++) {
        grid[i] = new char[size];
    }
}

And, this is my copy constructor that is causing the problem:

Grid::Grid(Grid const &other_grid) {
    size = other_grid.size;
    grid = new char *[other_grid.size];
    for(int i = 0; i < size; i++) {
        grid[i] = new char[size];
    }

    for(int i = 0; i < size; i++) {
        for(int j = 0; j < size; j++) {
            grid[i][j] = other_grid.grid[i][j];
        }
    }
}

Destructor

Grid::~Grid() {
for(int i = 0; i < size; i++) {
    delete [] grid[i];
}

delete [] grid;
}

operator = overloading

Grid & Grid::operator=(Grid const &other_grid) {
size = other_grid.size;
grid = new char *[other_grid.size];

for(int i = 0; i < other_grid.size; i++) {
    for(int j = 0; j < other_grid.size; j++) {
        grid[i][j] = other_grid.grid[i][j];
    }
}
return *this;
}

Shouldn't `size = grid_size;` read `size = other_grid.size;`? Please don't type your code here: *copy and paste it from actual failing code*, so we get to see code that only has the problems you're interested in. Also, why aren't you using `vector`s? — R. Martinho Fernandes, Jan 17 '12 at 02:26
@R.MartinhoFernandes, vectors make it easier, I know. But I'm trying to learn how to create these data structures without using the collection provided by C++. But this is the code I have right now I've corrected the mistake. — icanc, Jan 17 '12 at 02:37
That copy constructor looks okay (well, if you ignore exception safety, but that doesn't seem to be the problem here). Something else must be wrong somewhere. Perhaps in your destructor? Or copy-assignment operator? — R. Martinho Fernandes, Jan 17 '12 at 02:41
@R.MartinhoFernandes, would my operator= and destructor cause a problem even if they weren't invoked? I've pasted codes for those also. — icanc, Jan 17 '12 at 03:10
The operator= won't get invoked automatically, but the destructor will. It could be invoked without you noticing it. But the destructor looks fine :S (But the operator= is bad! It's leaking the previous memory and is not allocating a the new arrays for each row!). Can you post the test code that causes the failing copy? — R. Martinho Fernandes, Jan 17 '12 at 03:20
@R.MartinhoFernandes, Thanks for that tip and making me realize that I wasn't calling delete in the operator= function. That fixed everything and there were no memory leaks. — icanc, Jan 17 '12 at 03:38
Nice :) You may be interested in reading about the [copy-and-swap idiom](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom), which makes it easier to write the assignment operator. — R. Martinho Fernandes, Jan 17 '12 at 03:41
I definitely will. I love the fact that C++ allows you to do this. — icanc, Jan 17 '12 at 03:48

score 4 · Answer 1 · answered Jan 17 '12 at 02:30

Don't waste your time with that kind of manual allocation madness. Use std::vector.

class Grid {
    Grid(unsigned int size);

private:
    std::vector<std::vector<char>> grid;
};

Grid::Grid(unsigned int size)
: grid(size, std::vector<char>(size)) {}

And you get deallocation and working copies (and moves too, if you're using a modern compiler) for free.

brendanw · Answer 2 · 2012-01-17T07:40:40.283

EDIT: Re-read your code more carefully. Your assignment operator is broken. You're forgetting to allocate each row in the grid you're assigning to.

Separate point: You don't need all of those allocations. You only need one. Make grid a char* instead of char** and write it this way. I leave out checks for allocation failures here.

Grid::Grid(unsigned int grid_size)
    :size(grid_size), grid(0)
{
    if (size > 0)
    {
        grid = new char[size*size];
    }
}

Grid::Grid(Grid const &other_grid)
    :size(0)
{
    CopyFrom(other_grid);
}

Grid::~Grid() 
{
    if (size > 0)
    {
        delete [] grid;
        grid = 0;
    }
}

Grid& Grid::operator=(Grid const &other_grid) 
{
    CopyFrom(other_grid);
    return *this;
}

void Grid::CopyFrom(Grid const &other_grid)
{
    if (size > 0) delete [] grid;
    size = newSize;

    if (newSize > 0)
    {
        grid = new char[newSize*newSize];
        memcpy(grid, other_grid.grid, newSize*newSize);
    }
    else
    {
        grid = 0;
    }
}

Then if you want to access a byte in the grid at a point x, y, you can write it like this. (I'll leave appropriate bounds checking to you).

char Grid::GetByte(int x, int y)
{
    return grid[y*size + x];
}

Ugh, please don't implement the copy ctor in terms of the copy assignment operator. — Xeo, Jan 17 '12 at 06:45

Creating proper copy constructor for a dynamic 2d array

2 Answers2