Am I writing into memory I don't own?

Here is a very abridged excerpt from a simple matrix manipulation library I'm writing, mostly as a practice project.

Code:

---

class matrix {
public:
        double **a;
        int row, col;

    matrix (){};
    matrix (double *,int,int);
        matrix & operator = (const matrix &);
};

//constructor for matrix.
matrix::matrix(double *input, int inrow, int incol) {
        row = inrow;
        col = incol;
        a = new double*[row];

        for(int i=0;i<row;i++)
                a[i] = new double[col];

        if(input != NULL)
                for(int i=0;i<row;i++)
                        for(int j=0;j<col;j++)
                                a[i][j] = *(input + (col*i) +j);
        else
                for(int i=0;i<row;i++)
                        for(int j=0;j<col;j++)
                                a[i][j] = 0;
}

//Overloaded = operator, simply checks matrices are of equal size and then
//copies elements from right to left.
matrix & matrix::operator = (const matrix &rhs) {
        if(this == &rhs)
                return (*this); //not strictly necessary, I know.
        if(row == rhs.row && col == rhs.col){
                for(int i=0;i<row;i++)
                        for(int j=0;j<col;j++)
                                a[i][j]= rhs.a[i][j];
                return (*this);
        }
}

---

If I was to write this:

Code:

---

matrix test1 (some_array, rows, columns)
matrix test2 = test1; //here
matrix test3;
test3 = test1;//here

---

am I writing into memory I don't own?

It seems to me the the default constructor is called, which includes no memory allocation, and then I'm assigning data //here into memory I don't own.

However, I don't think I really understand the whole constructor business, and I don't get any errors at compile or runtime.

Define "don't own".

test1.a and test2.a will be the same value (i.e. pointing to the same memory). test3, being initialised with an empty constructor, will contain uninitialised data when it is created. So the assignment test3 = test1 (since it compares those uninitialised values with the values in test1) gives undefined behaviour.

As a rough rule, if your class allocates and manages memory, it is a good idea to write an appropriate copy constructor, an assignment operator, and a destructor. It is also a good idea to ensure constructors and assignment operators all behave consistently with each other: i.e. if one allocates memory, all of them do. Mixing and matching (some do, some don't) is error prone.

Your class does not follow "the rule of three" (look it up).
This line of code actually calls the copy constructor:

Code:

---

matrix test2 = test1; //here

---

But since you did not define one it will just make one by itself which would do the equivalent of a memcpy of the pointer value itself. That spell doom when the new object and the copied object are both destructed, when a double-free bug will occur.

ok, here is the code with a copy constructor, a destructor and memory allocation in the = operator

Code:

---

class matrix {
public:
        double **a;
        int row, col;

        matrix (){a=NULL;}; // is this good practice? I'm using it to identify to the = operator if memory needs allocating
        matrix (double *,int,int);
        matrix (const matrix &);
        ~matrix();
       
        matrix & operator = (const matrix &);
};

//constructor for matrix.
matrix::matrix(double *input, int inrow, int incol) {
        row = inrow;
        col = incol;
        a = new double*[row];

        for(int i=0;i<row;i++)
                a[i] = new double[col];

        if(input != NULL)
                for(int i=0;i<row;i++)
                        for(int j=0;j<col;j++)
                                a[i][j] = *(input + (col*i) +j);
        else
                for(int i=0;i<row;i++)
                        for(int j=0;j<col;j++)
                                a[i][j] = 0;
}

matrix::matrix (const matrix &in){
        row = in.row;
        col = in.col;

        for(int i=0;i<row;i++)
                a[i] = new double[col];

        for(int i=0;i<row;i++)
                for(int j=0;j<col;j++)
                        a[i][j] = in.a[i][j];
}

matrix::~matrix (){
        for(int i=0;i<row;i++)
        delete[] a[i];
        delete[] a;
}

//Overoaded = operator, simply checks matrices are of equal size and then
//copies elements from right to left.
matrix & matrix::operator = (const matrix &rhs) {
        if(this == &rhs)
                return (*this);
        if(a == NULL){
                row = rhs.row;
                col = rhs.col;

                for(int i=0;i<row;i++)
                        a[i] = new double[col];
        }


        if(row == rhs.row && col == rhs.col){
                for(int i=0;i<row;i++)
                        for(int j=0;j<col;j++)
                                a[i][j]= rhs.a[i][j];
                return (*this);
        }
}

---

does this represent anything close to good c++ practice?

Code:

---

class matrix {
public:
        double **a;
        int row, col;

        matrix (){**a=NULL;}; // is this good practice? I'm using it to identify to the = operator if memory needs allocating

---

You're dereferncing a when it doesn't point to anything. Perhaps you want a=NULL? Also, in C++ it is better to use 0 or nullptr instead of NULL. (nullptr was/is being reciently introduced to the language, so support may be limited).

Also, do you really want to allow default construction? It's not good practice to allow a partially built object. What is a matrix without data?

Code:

---

//Overoaded = operator, simply checks matrices are of equal size and then
//copies elements from right to left.
matrix & matrix::operator = (const matrix &rhs) {
        if(**a == NULL){
                row = rhs.row;
                col = rhs.col;

                for(int i=0;i<row;i++)
                        a[i] = new double[col];
        }
        if(this == &rhs)
                return (*this);

        if(row == rhs.row && col == rhs.col){
                for(int i=0;i<row;i++)
                        for(int j=0;j<col;j++)
                                a[i][j]= rhs.a[i][j];
                return (*this);
        }
}

---

What the two matrices are not the same size and the left is not empty?

Quote:

---

Originally Posted by King Mir

You're dereferncing a when it doesn't point to anything. Perhaps you want a=NULL? Also, in C++ it is better to use 0 or nullptr instead of NULL. (nullptr was/is being reciently introduced to the language, so support may be limited).

---

fixed it in my last edit. Isn't NULL just #DEF NULL 0? Surely there's no problem?

Quote:

---

Originally Posted by King Mir

What if the two matrices are not the same size and the left is not empty?

---

I still haven't worked out what to do with that, I haven't got my head around exceptions yet which I hope will prove to be the solution.

Quote:

---

Originally Posted by Skeksis

fixed it in my last edit. Isn't NULL just #DEF NULL 0? Surely there's no problem?

---

Because when you see NULL you assume that the type must be a pointer type. But there are situations with templates where either a pointer or an integer could be valid, and in such cases the language defaults to integer. Using NULL in such cases would obscure the ambiguity. Salem or someone like him will probably jump in any moment and provide a link with examples.

EDIT:Here's one

Code:

---

#include <iostream>
template<typename T> int foo(T eg){
  return 1;
}
template<typename T> int foo(T *eg){
  return 0;
}
int main(){
  std::cout << foo(NULL);//prints 1;
  return 0;
}

---

Quote:

---

I still haven't worked out what to do with that, I haven't got my head around exceptions yet which I hope will prove to be the solution.

---

You could destroy the current data, and reallocate everything.