0

I am writing a matrix class for a math project, and I have a strange problem where the copy constructor I've written fails in one function but succeeds in another- and the circumstances are seemingly identical. I've run it through gdb and the copy constructor executes but somehow just doesn't assign the values after it finishes.

Here is the output from gdb:

(gdb) break main.cpp:20
Breakpoint 1 at 0x8048913: file main.cpp, line 20.
(gdb) run
Starting program: /home/ian/Documents/math471/src/compress 
printing
1 1 1 1 
1 2 3 4 
1 3 5 7 
1 4 7 10 
printing
1 1 1 1 
1 2 3 4 
1 3 5 7 
1 4 7 10 
printing
1 1 1 1 
1 2 3 4 
1 3 5 7 
1 4 7 10 
printing
1 1 1 1 
1 2 3 4 
1 3 5 7 
1 4 7 10 

Breakpoint 1, main (argc=1, argv=0xbffff334) at main.cpp:20
20          tridiagonalize(A);
(gdb) s
tridiagonalize (A=...) at tridiagonalize.cpp:10
10          Matrix result(A);
(gdb) print result
$1 = {transposed = false, height = 3903476, width = 0, data = 0x0}
(gdb) s
Matrix::Matrix (this=0xbffff23c, A=...) at matrix.cpp:176
176         height = A.getHeight();
(gdb) print height
$2 = 0
(gdb) n
177         width = A.getWidth();
(gdb) print height
$3 = 4
(gdb) n
178         data = new double*[height];
(gdb) print width
$4 = 4
(gdb) n
179         for(int i = 0; i < height; i++){
(gdb) break matrix.cpp:185
Breakpoint 2 at 0x80492ba: file matrix.cpp, line 185.
(gdb) c
Continuing.

Breakpoint 2, Matrix::Matrix (this=0xbffff23c, A=...) at matrix.cpp:185
185         transposed = false;
(gdb) n
186 }
(gdb) print transposed
$5 = false
(gdb) print this
$6 = (Matrix * const) 0xbffff23c
(gdb) print this->height
$7 = 4
(gdb) n
tridiagonalize (A=...) at tridiagonalize.cpp:11
11          int n = A.getWidth();
(gdb) print result
$8 = {transposed = false, height = 3903476, width = 0, data = 0x0}
(gdb) 

As you can see, the values in result are the same before the constructor as after.

#include "matrix.h"
#include "tridiagonalize.h"


using namespace std;

void function(Matrix &A);

int main(int argc, char** argv){

        Matrix A(4, 4);
        for(int i = 0; i < 16; i++){
                A.set(i/4, i%4, (i%4)*(i/4)+1);
        }
        function(A);
        Matrix B = A;
        A.print();
        B.print();

        tridiagonalize(A);
        //A.print();

        return 0;
}

void function(Matrix &A){
        Matrix result(A);
        A.print();
        result.print();
}

Matrix tridiagonalize(Matrix &A){

        Matrix result(A);
        int n = A.getWidth();
        cout << "width: " << n << endl;
        cout << "width of result: " << result.getWidth() << endl;
            return result;
}

EDIT: here is the matrix code:

void Matrix::set(int i, int j, double value){
        if(transposed){
                int tmp = i;
                i = j;
                j = tmp;
        }
        if(i < 0 || j < 0 || i >= height || j >= width){
                cout << "trying to set value outside of matrix" << endl;
                return;
        }
        if(height > 0 && width > 0){
                data[i][j] = value;
        }
        return;
}

void Matrix::print() const{
        cout << "printing" << endl;
        if(!transposed){
                for(int i = 0; i < height; i++){
                        for(int j = 0; j < width; j++){
                                cout << data[i][j] << " ";
                        }
                        cout << endl;
                }
        }
        else{
                for(int i = 0; i < width; i++){
                        for(int j = 0; j < height; j++){
                                cout << data[j][i] << " ";
                        }
                        cout << endl;
                }
        }
        return;
}

Matrix::~Matrix(){
        //cout << "deleting Matrix" << endl;
        if(data != NULL){
                for(int i = 0; i < height; i++){
                        //cout << data[i] << endl;
                        delete[] data[i];
                }
                //cout << data << endl;
                delete[] data;
        }
}

double Matrix::get(int i, int j) const {
        if(transposed){
                int tmp = i;
                i = j;
                j = tmp;
        }
        if(data != NULL && i >=0 && j >= 0 && i < height && j < width){
                return data[i][j];
        }
        else{
                cout << "error in get" << endl;
        }
        return -1;
}

int Matrix::getHeight() const {
        if(transposed){
                return width;
        }
        else{
                return height;
        }
}

int Matrix::getWidth() const {
        if(transposed){
                return height;
        }
        else{
                return width;
        }
}



Matrix::Matrix(const Matrix& A){
        height = A.getHeight();
        width = A.getWidth();
        data = new double*[height];
        for(int i = 0; i < height; i++){
                data[i] = new double[width];
                for(int j = 0; j < width; j++){
                        data[i][j] = A.get(i,j);
                }
        }
        transposed = false;
}

Matrix Matrix::operator= (Matrix &B){
        if(data != NULL){
                for(int i = 0; i < height; i++){
                        delete[] data[i];
                }
                delete[] data;
        }
        data = NULL;
        Matrix result(B);
        return result;
}

The tridiagonalize function and the 'function' function are set up the same, but function() works and tridiagonalize() doesn't. Any ideas why this is happening?

EDIT: added the matrix code

Ian
  • 599
  • 2
  • 5
  • 13
  • Eventually it could be because of the return type Matrix... ? – evotopid Dec 11 '11 at 20:19
  • added the matrix code. Thank you for the help. – Ian Dec 11 '11 at 20:23
  • Make a testcase that reproduces the problem, please, not a half-dump of code (most of which is irrelevant, and some of which is even commented-out!) This is the result of your own divide-and-conquer debugging which you performed to narrow down the issue to <30 lines before asking. – Lightness Races in Orbit Dec 11 '11 at 20:24
  • Side note: that `operator=` code just isn't going to work like that. – Neil Dec 11 '11 at 20:33

1 Answers1

2

Matrix::operator=() isn't working correctly, it frees the old data array but then doesn't create a new one with the data that should be assigned.

It should rather look like this:

Matrix& Matrix::operator= (const Matrix &B){
   // Don't do anything if B is the same object
   if (&B == this)
      return *this;

   // delete data, as in the current version
   if(data != NULL){
      ...
   }

   // Create new array of arrays, fill with numbers from B
   data = new double*[B.getHeight()];
   ...

   return *this;
}
sth
  • 222,467
  • 53
  • 283
  • 367
  • 1
    Would be nice to also check for self-assignment (http://www.parashift.com/c++-faq-lite/assignment-operators.html#faq-12.1) – Drahakar Dec 11 '11 at 20:37
  • Thank you. I fixed the assignment operator. I don't think that fixes the copy constructor problem though. – Ian Dec 11 '11 at 20:44
  • Well wait, maybe it did. Thank you. – Ian Dec 11 '11 at 20:46
  • @Ian: Are you familiar wit hthe [copy-and-swap](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) idiom? – Benjamin Lindley Dec 11 '11 at 20:51
  • @Ian: Great, I was hoping for that :). Since the assignment operator used the copy constructor internally it was probably confusing where the error really came from. – sth Dec 11 '11 at 20:55