Bad Access Error in 2D-Array (Matricies)

Question

I have a little bit of a problem... I understand what a EXC_BAD_ACCESS error is and I generally know how to fix it but this one has got me completely stuffed. I have this all within a class, here is one method:

double Matrix::get_element(int r, int c) const {
    //Retrieve the element at row r and column c
    //Should not modify the value stored in Matrix but return a double copy of the value

    double currentValue = matrix[r][c];
    return currentValue;
}

Now, I have another piece of my code that calls this method:

std::string Matrix::to_string() const {
    std::string result;
    double current;
    Matrix working = *this;
    std::ostringstream oss;

    oss << "[";
    for (int i = 0; i < rows; i++) {
        for (int j = 0; j < cols; j++) {
            current = 0.0;
            current = working.get_element(i, j);
            oss << " " << current << " ";
        }
        oss << "; ";
    }
    oss << "]";
    result = oss.str();
    return result;
}

I know that the working object has 3 rows and 3 cols at the point where working.get_element(i, j); is called. The variable list shows me just before the get_element() method, that both rows and cols are set to 3. In the method, I'm able to get the value at get_element(0, 0) but not get_element(0, 1).

I can't see why this is the case... Anyone know why or require more of my code to understand why these methods are being called?

EDIT: Here is the header file:

class Matrix {
private:
    //Any variables required
    int rows;
    int cols;
    double **matrix;

public:
    Matrix();   //Working M
    ~Matrix();  //Working M
    Matrix(int r, int c);   //Working M

    int getRows();
    int getCols();

    void set_element(int r, int c, double val); //Working M
    double get_element(int r, int c) const; //Working M

    void clear(); //Working M        
    bool is_empty(); //Working M         
    bool is_identity(); //Working M

    const Matrix transpose(); //Working M
    int minorMat(double **dest, const int row, const int col, int order); //Working M
    double get_determinent(); //Working M
    double higherDeterminents(int order); //Working M

    const Matrix operator+(const Matrix &rhs); //Working M       
    const Matrix operator-(const Matrix &rhs); //Working M    
    const Matrix operator*(const Matrix &rhs); 
    bool operator==(const Matrix &rhs); //NOT assessed
    const Matrix operator*(const double &rhs);        
    const Matrix operator/(const double &rhs);
    Matrix & operator=(const Matrix &rhs);

    std::string to_string() const;
};

Do ignore the comments sorry. And this is the constructors/destructors:

Matrix::Matrix() {
    //Basic Constructor
    rows = 1;
    cols = 1;
    matrix = new double*[rows];
    for (int i = 0; i < rows; ++i) {
        matrix[i] = new double[cols];
    }
}

Matrix::~Matrix() {
    //Basic Deconstructor
    for (int i = 0; i < rows; ++i) {
        delete[] matrix[i];
    }
    delete[] matrix;
    rows = NULL;
    cols = NULL;
    matrix = NULL;
}

Matrix::Matrix(int r, int c) {
    //Empty matrix (all 0's) with r rows and c columns, if they are -ve, set to 1
    rows = r;
    cols = c;

    if (cols < 0)
        cols = 1;
    if (rows < 0)
        rows = 1;

    matrix = NULL;
    matrix = new double*[rows];
    for (int i = 0; i < rows; i++) {
        matrix[i] = new double[cols];
    }
}

EDIT2:

Matrix & Matrix::operator=(const Matrix &rhs) {
    //rhs is matrix to be copied
    //rhs compied into Matrix called on
    double toCopy;
    for (int i = 0; i < rhs.rows; i++) {
        for (int j = 0; j < rhs.cols; j++) {
            toCopy = rhs.get_element(i, j);
            this->set_element(i, j, toCopy);
        }
    }
    return *this;
}

Answer 1

Your class is violating the "big three" rule. If a class has one of a destructor, assignment operator or copy constructor then it most probably you need to have all three.

In your case you have a destructor, but no assignment operator or copy constructor, and this is going to create an UB condition when you do Matrix working = *this.

By the way there are two other problems with your code

1. From the comment seems that you think that new double[size] will initialize the elements to 0 and this is not true.

2. Most of your code is technically very bad. Your matrix class would be much easier (less code) to implement correctly using std::vector instead of pointers and dynamic memory. Of course if this is just an exercise then it make sense to avoid using std::vector.

By the way if you never heard about the "big three" rule chances are that you're trying to learn C++ with experimentation instead that by reading.

With C++ this is not a smart move... logic can be used as substitute for study if 1) the topic is highly logical, 2) if you can be told when you get it wrong.

Instead C++ is very very complex and in a few place is also quite illogical (for historical reasons) so there are parts in which logic will simply misguide you.

Moreover when you make a mistake in C++ you don't get in general an error message, but "Undefined Behavior". This basically makes very very hard to learn C++ with experimentation because even wrong code may apparently work. It is also very easy to write code that looks good and that is instead quite wrong for subtle reasons.

Instead of just experimenting you should grab a good book and read it cover to cover...

Answer 2

Your Matrix(int r, int c) allocates memory for matrix, but leaves the values it points to uninitialized. Add something like this to the constructor:

for (int i = 0; i < r; i++) {
        for (int j = 0; j < c; j++) {
            this->set_element(i, j, 0);
        }
    }

Doing like this:

int main()
{
    Matrix m(3,3);
    std::cout << m.to_string();
}

Output:

[ 0 0 0; 0 0 0; 0 0 0; ].

Same thing in the default constructor:

Matrix::Matrix() {
    //Basic Constructor
    rows = 1;
    cols = 1;
    matrix = new double*[rows];
    for (int i = 0; i < rows; ++i) {
        matrix[i] = new double[cols];
    }
}

You allocated memory, but wherever matrix[0][0] points to, it's uninitialized garbage value. Do something like matrix[0][0] = 0; or whatever default value you want it to have.

Hope that helps.

Answer 3

It is impossible for us to say when you do not state how you declare and initialize the matrix element. Using something like that in your CTOR should be fine:

class Matrix {
   float matrix[3][3];
   ...
}

Don't forget to initialize it in your CTOR to something that makes sense.

Btw: why do you do this: Matrix working = *this; ?? You could simply this->get_element(i, j); instead, which would not invoke copying of your whole object. [1]

EDIT: Update since you updated your answer. You should be careful with your copy CTORs and operator=() statements. It is easily possible to make double deletes or something ugly like that.

EDIT2: I think the problem is this line:

Matrix working = *this;

You are creating a new copy working of your this object. But working gets initialized with only 1 column and 1 row (as defined in your standard CTOR). I'm not sure if you are checking the bounds when calling set_element or get_element so I guess you are writing over the bounds of your arrays.

I think the best idea is to just remove the line Matrix working = *this; and to adhere to my tip in above: this->get_element(i, j); in std::string Matrix::to_string() const.