0

My programming assignment wanted me to make a class with a copy constructor and to overload the assignment operator (=). My code runs smoothly until I use the overloaded assignment operator, then it returns an error, segmentation fault (core dumped).

When I change it to use the copy constructor it doesn't return any errors, so I assume the problem is within the operator overloading part. Any idea what caused it? If it is the operator overloading part, what's the problem? Any help would be appreciated, thanks.

#define DefRow 2
#define DefCol 2
typedef double* DouArray;

class TwoD{
public:
    TwoD();
    TwoD(const TwoD& one);      //Copy Constructor
    ~TwoD();
    void setElement(int row, int column);
    double getElement(int row, int column);
    int getRows();
    int getCols();
    TwoD operator =(const TwoD& a);
private:
    double** Matrix;
    int MaxRows, MaxCols;
};

TwoD::TwoD(void){
    MaxRows = DefRow;
    MaxCols = DefCol;
    Matrix = new DouArray[MaxRows];
    for(int i = 0; i < MaxRows; i++){
        Matrix[i] = new double[MaxCols];
    }
}

TwoD::TwoD(const TwoD& one){
    MaxRows = one.MaxRows;
    MaxCols = one.MaxCols;
    Matrix = new DouArray[MaxRows];
    for(int i = 0; i < MaxRows; i++){
        Matrix[i] = new double[MaxCols];
    }
    for(int i = 0; i < MaxRows; i++){
        for(int j = 0; j < MaxCols; j++){
            Matrix[i][j] = one.Matrix[i][j];
        }
    }
}

TwoD::~TwoD(void){
    for(int i = 0; i < MaxRows; i++){
        delete [] Matrix[i];
    }
    delete [] Matrix;
    Matrix = NULL;
    MaxRows = 0;
    MaxCols = 0;
}

void TwoD::setElement(int row, int column){
    cin >> Matrix[row][column];
}

double TwoD::getElement(int row, int column){
    return Matrix[row][column];
}

int TwoD::getRows(){
    return MaxRows;
}

int TwoD::getCols(){
    return MaxCols;
}

operator overload here

TwoD TwoD::operator =(const TwoD& a){
    MaxRows = a.MaxRows;
    MaxCols = a.MaxCols;
    for(int i = 0; i < MaxRows; i++){
        for(int j = 0; j < MaxCols; j++){
            Matrix[i][j] = a.Matrix[i][j];
        }
    }
}

main here

int main(){
    int inrow, incol;
    cout << "Enter the row and column dimensions of the array." << endl;
    cin >> inrow >> incol;
    TwoD matrix2(inrow, incol);
    cout << "Enter " << inrow << " rows of " << incol << " doubles each." << endl;
    for(int i = 0; i < inrow; i++){
        for(int j = 0; j < incol; j++){
            matrix2.setElement(i, j);
        }
    }
    TwoD temp2 = matrix2;
    cout << "Echoing the 2 dim. array, matrix2" << endl;
    for(int i = 0; i < temp2.getRows(); i++){
        for(int j = 0; j < temp2.getCols(); j++){
            cout << temp2.getElement(i, j) << " ";
        }
        cout << endl;
    }
    cout << "Assigning matrix2 to matrix3" << endl;
    TwoD matrix3;

    matrix3 = matrix2; //I got the error here

    cout << "Displaying the 2 dim. array, matrix3 resulting from assignment" << endl;
    cout << "Rows " << matrix3.getRows() << " Cols " << matrix3.getCols() << endl;
    for(int i = 0; i < matrix3.getRows(); i++){
        for(int j = 0; j < matrix3.getCols(); j++){
            cout << matrix3.getElement(i, j) << " ";
        }
        cout << endl;
    }
    return 0;
}

Expected result:

Enter the row and column dimensions of the array
3 4
Enter 3 rows of 4 doubles each.
1.0 1.0 1.0 1.0
2.0 2.0 2.0 2.0
3.0 3.0 3.0 3.0
Echoing the 2 dim. array, matrix2
1 1 1 1
2 2 2 2
3 3 3 3
Assigning matrix2 to matrix3
Displaying the 2 dim. array, matrix3 resulting from assignment
Rows 3 Cols 4
1 1 1 1
2 2 2 2
3 3 3 3

Actual result:

Enter the row and column dimensions of the array.
3 4
Enter 3 rows of 4 doubles each.
1.0 1.0 1.0 1.0
2.0 2.0 2.0 2.0
3.0 3.0 3.0 3.0
Echoing the 2 dim. array, matrix2
1 1 1 1
2 2 2 2
3 3 3 3
Assigning matrix2 to matrix3
Segmentation fault (core dumped)
yacc
  • 2,915
  • 4
  • 19
  • 33
  • 2
    How `TwoD` is defined? Please provide [mcve] – kiran Biradar Jun 04 '19 at 10:20
  • 2
    That won't compile since you don't have a default constructor. – molbdnilo Jun 04 '19 at 10:36
  • 1
    Sh*t, sorry... my head is in the cloud... wait, let me edit it – Hatsune Miku Jun 04 '19 at 10:38
  • I haven't looked at the rest of the code, but `Matrix[i][j] = ...` potentially writes out of bounds. Your code doesn't make sure that `Matrix[i]` and `Matrix[i][j]` actually exist. – melpomene Jun 04 '19 at 10:39
  • `prog.cpp:17:15: error: ‘DefRow’ was not declared in this scope` – melpomene Jun 04 '19 at 10:50
  • You need to adjust the allocated dimensions in the assignment operator and copy constructor. – molbdnilo Jun 04 '19 at 10:53
  • @melpomene oh yeah, I used #define for DefRow and DefCol, just change it to random integer – Hatsune Miku Jun 04 '19 at 11:12
  • @molbdnilo How do you do that? – Hatsune Miku Jun 04 '19 at 11:12
  • `prog.cpp:19:18: error: ‘DouArray’ does not name a type` – melpomene Jun 04 '19 at 11:22
  • @HatsuneMiku You should post code that will compile without *any* changes from us. You are missing `#include` statements, this line: `TwoD matrix2(inrow, incol);` could never compile since your class lacks a constructor that has two arguments. Given all of this, there are much better ways to create a 2D array that will shorten your code by a lot. [See this](https://stackoverflow.com/questions/21943621/how-to-create-a-contiguous-2d-array-in-c/21944048#21944048). Your implementation is one of the worst ways of creating a uniform (rows have the same number of columns) matrix. – PaulMcKenzie Jun 04 '19 at 12:26
  • @HatsuneMiku The reason why it is bad is because you are allocating each row separately. That not only is slower, it fragments memory, and is not cache friendly. As to the fix, all you really need to concentrate on is to fix the copy constructor. Once you do that, the assignment operator is a 4 line function of nothing but calls to `std::swap` like this `TwoD operator =(const TwoD& a){ TwoD temp(a);std::swap(temp.MaxRows, MaxRows); std::swap(temp.MaxCols, MaxCols);std::swap(temp.Matrix, Matrix); return *this;}` – PaulMcKenzie Jun 04 '19 at 12:27
  • @PaulMcKenzie About the code I posted, sorry, this is my second(?) time posting here, the first one was quite a while ago, please forgive me if my post lacks some required lines, I'll include them next time. Thanks for your input. :) – Hatsune Miku Jun 04 '19 at 12:52
  • @PaulMcKenzie For the 2D array part (allocating each row separately), I know it's bad. The example I got is doing it like that, so I just copy pasted that part. At first, I was confused, I have read about it being bad and looked for another way to do it. I found one (not as long as the one you linked me to) and tried to implement it to my code, but it doesn't work, it return a bunch of errors about memory address iirc, looked around and found nothing, so I just kinda stick with it. Might test the one you linked me to. – Hatsune Miku Jun 04 '19 at 12:53
  • @PaulMcKenzie Oh, and I replaced my operator overloading part with the one you said, using the calls to "swap"... and it worked, I didn't change my copy constructor, but I'm not sure if it's 100% correct.... Anyway, thanks for your help, really really appreciate it... – Hatsune Miku Jun 04 '19 at 12:55

0 Answers0