c++ segmentation fault when deleting dynamic tables

Question

everyone I have to make a dynamic matrix and here are the constructors and destructor I have:

Board::Board() {
    a_l=0;
    a_c=0;
    a_mp=NULL;
}
Board::Board(const Board&t) {
    a_l=t.a_l;
    a_c=t.a_c;
    a_mp=t.a_mp;
    Memory();
    copy(t);
}
Board::Board(int nl, int nc) {
    a_l=nl;
    a_c=nc;
    Memory();
}
Board::~Board() {
    freeMemory();
}

// PRIVATE METHODS

void Board::copy(const Board &t) {
    int a_l, a_c;
    int ** a_mp;
    a_l=t.a_l;
    a_c=t.a_c;
    for(int i=a_l;i<a_c;i++) {
        for(int j=a_c;j<a_l;j++) {
            a_mp[i][j]=t.a_mp[i][j];
        }
    }
}
void Board::freeMemory() {
    for(int i=0;i<a_l-1;i++) {
        delete [] a_mp[i];
    }
    delete [] a_mp;
}
void Board::Memory() {
    char ** a_mp;
    a_mp = new char*[a_l];
    for(int i =0;i<a_l; i++) {
        a_mp[i]=new char[a_c];
        for(int j=0;j<a_c;j++)
            a_mp[i][j]='-';
    }
}

Board is the class and a_l and a_c are number of lines and columns of the matrix. In my main, I declare a Board variable and then I do this:

board=Board(5,5);

It compiles, but when I want to display it, like this for example:

cout << board.Cols() << endl;

This is the method:

int Board::Cols() const {
    return (a_c);
}

It displays 0. As if it didn't create board with the parameters I said. Also the program crashes when I do this board=Board(5,5); so I use the debugger and it says it stops at this line of the delete:

board=Board(5,5);

I don't know why it crashes and I don't know why it doesn't keep the values of the board variable I've declared! Anyone knows why?

EDIT: rMemory=Memory, it was a type from here not from the program

board=Board(5,5); wouldnt work, this is c++, instead Board board = new Board(5,5); what types are a_c and a_l? — Ariel Pinchover, Apr 18 '13 at 13:53
They are both int. @tacp, Memory=rMemory, it was a typo copying the code — p. bosch, Apr 18 '13 at 13:55
@soon first off, he didnt declare its type (or he didnt mention it), secondly to dynamically allocate space you use new. — Ariel Pinchover, Apr 18 '13 at 13:58
@Infested, 1) "_I declare a Board variable_" - so, type of `board` is `Board`, I guess, not `Board*`. 2) Why are you using dynamic allocation in that case? — awesoon, Apr 18 '13 at 14:02
@soon the title is dynamic tables. actually, if an object is instantiated not dynamically then its destructor is built in. since you coded this class, and you dont know whats going to be in it when you start the program you allocate it — Ariel Pinchover, Apr 18 '13 at 14:03
@soon well that explains. in freeMemory watch for how you delete, i know that its very strict with how it should be written with arrays. delete[] != delete [] — Ariel Pinchover, Apr 18 '13 at 14:08
@Infested, I see no differences between `delete[]` and `delete []`, do you? — awesoon, Apr 18 '13 at 14:12
let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/28446/discussion-between-soon-and-infested) — awesoon, Apr 18 '13 at 14:18
Note: Don't add "solved" and "thank you" to the question once answered. That's what the answer checkmark and upvotes are for. — Anthony Pegram, Apr 18 '13 at 14:51
just updating here my answer about delete[] vs delete []: delete is a token the tokenizer (when building the project) is looking for tokens since delete [] is delete space [] it counts delete as a token, then [] as a token whilst delete[] is another token — Ariel Pinchover, Apr 18 '13 at 15:33

stardust · Answer 1 · 2013-04-18T14:04:49.347

3

int ** a_mp;

Needs to be just a_mp. Otherwise you are declaring a new variable. Instead of using the member one.

And then it needs to be allocated to. right now it is not.

void Board::copy(const Board &t) {
    int a_l, a_c;
    a_mp = new char[t.a_l][t.a_c];
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    // a_l=t.a_l; These have already be done in the copy constructor. 
    // a_c=t.a_c;
    for(int i=a_l;i<a_c;i++) {
        for(int j=a_c;j<a_l;j++) {
            a_mp[i][j]=t.a_mp[i][j];
        }
    }

}

or if you want you can allocate it in the copy constructor

Board::Board(const Board&t) {
    a_l=t.a_l;
    a_c=t.a_c;
    // a_mp=t.a_mp;  // This is wrong!!
    a_mp = new char[t.a_l][t.a_c];
    copy(t);
}

edited Apr 18 '13 at 14:04

answered Apr 18 '13 at 13:52

stardust

5,918
1
18
20

But I need to make a copy of the actual object, thats why I have the method copy – p. bosch Apr 18 '13 at 13:56
@p.bosch Yes you need the copy() to copy the data. That didn't change. I commented the column and row assignments because you have already done them in `Board::Board(const Board&t)`before you call copy() – stardust Apr 18 '13 at 13:57
I've done this, but now I'm getting 2 errors on the line a_mp = new int[t.a_l][t.a_c]; which are 't' and '.' cannot appear in constant-expression. What do they mean? – p. bosch Apr 18 '13 at 13:59
@p.bosch how did you declare a_mp in the class? – Ariel Pinchover Apr 18 '13 at 14:01
These are the attributes in the class: private: int a_l; int a_c; char ** a_mp; – p. bosch Apr 18 '13 at 14:02
@p.bosch the matrix is char? Oh :) I will edit the answer. I just assumed it was int. And if you can please edit and put the errors on the question. – stardust Apr 18 '13 at 14:03
@user2280716 I automatically changed that when trying it out and it didn't work out. Salgar has solves my crashing problem but values still don't get saved, any idea? – p. bosch Apr 18 '13 at 14:21
@p.bosch yes remove `a_mp=t.a_mp;` from `Board::Board(const Board&t)` and remove `int ** a_mp;` from `Board::copy(const Board &t)` – stardust Apr 18 '13 at 14:24

Useless · Accepted Answer · 2013-04-18T14:53:35.430

Firstly, this assignment

board=Board(5,5);

is over-complicated. You can just declare

Board board(5,5);

directly (the extra work may well be optimized out, but it's un-idiomatic).

Secondly, everywhere you do something like:

void Board::copy(const Board &t) {
    int a_l, a_c;
    int ** a_mp;

you're shadowing the object's member variables. That is, a_l now refers to the local integer variable inside this function, instead of the object member. You'll have to refer to this->a_l if you want to see or change the object.

Now, the copy constructor

Board::Board(const Board&t) {
    a_l=t.a_l;
    a_c=t.a_c;
    a_mp=t.a_mp;
    Memory();
    copy(t);
}

does:

a shallow copy (it shares the a_mp pointer with the object you're copying)
calls Memory which allocates a new lump of memory, referred to as a_mp only inside Memory, and then leaked (when that local variable goes out of scope)
calls copy, which copies the original objects values into an uninitialized local pointer also called a_mp (this is undefined behaviour, because those writes could go anywhere)
ends up with the value of a_mp it first chose, so now two Board instances have the same a_mp value, and it will be freed twice.

Here's some sample code that fixes those problems: I've changed the structure of your code as little as possible (bar renaming things for readability). There's plenty of room for improvement, but it should at least be correct.

class Board {
    int rows;
    int cols;
    char **data;
public:
    Board(): rows(0), cols(0), data(0) {}
    Board(int nl, int nc) : rows(nl), cols(nc) 
    {
        allocate_data();
    }
    Board(const Board& other)
        : rows(other.rows), cols(other.cols)
    {
        allocate_data();
        copy_data(other);
    }
    ~Board() {
        free_data();
    }
private:
    void copy_data(const Board &other) {
        for(int r=0; r<rows; r++)
            for(int c=0; c<cols; c++)
                data[r][c]=t.data[r][c];
    }
    void free_data() {
        for(int r=0; r<rows; r++)
            delete [] data[r];
        delete [] data;
    }
    void allocate_data() {
        data = new char*[rows];
        for(int r=0; r<rows; r++) {
            data[r]=new char[cols];
            for(int c=0; c<cols; c++)
                data[r][c]='-';
        }
    }
};

Note that this works fine if you only use the copy constructor, but the default-generated assignment operator will still be wrong. As Daniel Weber pointed out in a comment, the rule of three suggests you ought to write this as well:

    Board& operator=(const Board& other) {
        free_data();
        rows = other.rows;
        cols = other.cols;
        allocate_data();
        copy_data(other);
    }

Note that the copy assignment operator needs to cope with a destination object which is already initialized, and may not have the right dimensions. You could improve this to only re-allocate if the new (other) board is larger.

If you have C++11 support, you can also add the move equivalents of the copy constructor and assignment operator:

    Board(Board&& original)
        : rows(original.rows), cols(original.cols)
    {
        data = original.data;
        original.data = NULL;
    }
    Board& operator=(Board&& original) {
        free_data();
        rows = original.rows;
        cols = original.cols;
        data = original.data;
        original.data = NULL;
    }

This answer lacks the Rule of Three (http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). The problem of the missing assignment operator is bypassed by using the constructor instead. — Pixelchemist, Apr 18 '13 at 14:33

score 1 · Answer 3 · edited May 23 '17 at 11:57

for(int i=0;i<a_l-1;i++) in freeMemory runs from i=0 to i=3, which is the last i that is < 5-1. You are missing one line since your allocation Loop runs is for(int i =0;i<a_l; i++) and therefore runs from i=0 to i=4. You allocate one more line.

Another thing is: What does copy() do? You copy the values of t.a_l and t.a_c to temporary variables which are deleted once copy ends and you assign values to unallocated Memory (the temporary **a_mp). Remove the declaration and assignments within this function and only leave the a_mp data copy.

void Board::copy(Board const &t) {
    for(int i=a_l;i<a_c;i++) {
        for(int j=a_c;j<a_l;j++) {
            a_mp[i][j]=t.a_mp[i][j];
        }
    }
}

What i did:

Remove the declaration of a_mp in Memory()
Add a Assignment Operator -> What is The Rule of Three?
Check for NULL-pointer in the freeMemory() function

Looks like this:

class Board
{
public:
  int a_l, a_c;
  char ** a_mp;

  Board() : a_l(0), a_c(0), a_mp(NULL) 
  {
  }
  Board(const Board&t) : a_l(t.a_l), a_c(t.a_c), a_mp(NULL) 
  {
    Memory();
    copy(t);
  }
  Board(int nl, int nc) : a_l(nl), a_c(nc), a_mp(NULL) 
  {
    Memory();
  }

  Board& operator= (Board const &t)
  {
    freeMemory();
    a_l = t.a_l;
    a_c = t.a_c;
    Memory();
    copy(t);
    return *this;
  }

  Board::~Board() 
  {
    freeMemory();
  }

  // PRIVATE METHODS

  void copy(const Board &t) 
  {
    for(int i=a_l;i<a_c;i++) 
    {
      for(int j=a_c;j<a_l;j++) 
      {
        a_mp[i][j]=t.a_mp[i][j];
      }
    }
  }
  void freeMemory() 
  {
    if (a_mp == NULL)
    {
      for(int i=0;i<a_l;i++) 
      {
        delete [] a_mp[i];
      }
      delete [] a_mp;
    }
  }
  void Memory() {
    a_mp = new char*[a_l];
    for(int i =0;i<a_l; i++) 
    {
      a_mp[i]=new char[a_c];
      for(int j=0;j<a_c;j++) a_mp[i][j] = '-';
    }
  }

  int Cols() const 
  {
    return (a_c);
  }

};

Works.

Board testboard;
testboard = Board(5,5);
cout << "Cols are: " << testboard.Cols() << endl;

Prints: "Cols are: 5".

score 1 · Answer 4 · answered Apr 18 '13 at 14:02

1

void Board::Memory() {
    char ** a_mp;
    a_mp = new char*[a_l];
    for(int i =0;i<a_l; i++) {
        a_mp[i]=new char[a_c];
        for(int j=0;j<a_c;j++)
            a_mp[i][j]='-';
    }
}

You're declaring a local variable on the stack called a_mp. This pointer points to all of the allocated memory on the heap. Then it goes out of scope at the end of the call to Memory(). Now you have no way of accessing any of that memory you just assigned. This is sad. a_mp should be a a member variable, so that you still have a reference to your data after Memory finishes. And so that your destructor knows what memory to free.

i.e. Remove this line: char ** a_mp;

answered Apr 18 '13 at 14:02

Salgar

7,687
1
25
39

May I also suggest that you remove the copy and the copy constructor functions, and just get it working with the normal constructor first. i.e. write `Board foo(5,5); foo.cols(); foo.stuff(); foo.anything_else();` and make sure it also exits cleanly without crashing. Then afterwards add the copy function. As you have so many different errors here apparently. I just dealt with the first one. – Salgar Apr 18 '13 at 14:07
Yes! It works. But I'm still getting the second error: cout << board.Cols() << endl; This doesnt display the actual columns, it displays 0. I'm calling it right after the board=Board(5,5) – p. bosch Apr 18 '13 at 14:16
@p.bosch try this function without the const in the end of it. – Ariel Pinchover Apr 18 '13 at 14:17
@Infested If you meant board.Cols() I tried it but no difference, still doesn't save the values. – p. bosch Apr 18 '13 at 14:19
This is the same problem for your copy() function. Remove ` int a_l, a_c;` and make sure they are members and Cols() will work. Otherwise, stop using the copy constructor, and it will work if you declare your variable like I did above. – Salgar Apr 18 '13 at 14:30

c++ segmentation fault when deleting dynamic tables

4 Answers4