-1

So I'm practicing coding in c++ and I'm attempting to write a class for matrices (stored as arrays) with the relevant overloaded operations.

I've got as far as defining the class and attempting to overload the << operator but my current code is causing a segmentation fault (I compile with g++ in Ubuntu). I've looked online and people with similar problems tend to always forget to return os in their overload function, but I've done that so I have no idea what my problem could be. Furthermore, my overloaded operator works several times before it causes a segmentation fault.

Any help would be greatly appreciated.

Here is my code:

#include<iostream>
#include<stdlib.h> // for c style exit
using namespace std;

class matrix
{
  // Friends
  friend ostream & operator<<(ostream &os, const matrix &mat);
  friend istream & operator>>(istream &is, matrix &mat);

private:
  double *mdata;
  int rows,columns;
public:
  // Default constructor
  matrix(){mdata=0; rows=columns=0;}
  // Parameterized constructor
  matrix(int m, int n){mdata = new double[ m*n ]; rows = m; columns = n;}
  // Copy constructor
  matrix(matrix &mat)
  // Destructor
  ~matrix(){delete[] mdata; cout<<"Destructing array."<<endl;}
  // Access functions
  int getrows() const {return rows;} // Return number of rows
  int getcols() const {return columns;} // Return number of columns
  int index(int m, int n) const // Return position in array of element (m,n)
  {
    if(m>0 && m<=rows && n>0 && n<=columns) return (n-1)+(m-1)*columns;
    else {cout<<"Error: out of range"<<endl; exit(1);}
  }
  double & operator()(int m, int n)const {return mdata[index(m,n)];}
  // Other access functions go here
  double & operator[](int i) {return mdata[i];}
  // Other functions 
  // Copy  Assignment operator
  matrix & operator=(matrix &mat);
};

// Member functions defined outside class
matrix::matrix(matrix &mat){
  rows = mat.getrows();
  columns = mat.getcols();
  for(int j = 0; j<rows*columns; j++){mdata[j] = mat[j];}
 }

matrix & matrix::operator=(matrix &mat){
  if (&mat == this) return *this;

  delete[] mdata; rows = 0; columns = 0;

  rows = mat.getrows(); columns = mat.getcols();
  if(rows>0&&columns>0){
    mdata = new double[(columns-1) + (rows-1)*columns + 1];
    for(int j = 0; j<rows*columns; j++){mdata[j] = mat[j];}
  }
  return *this;
}


// Overload insertion to output stream for matrices
ostream & operator<<(ostream &os, const matrix &mat){
  for(int j = 0;j<mat.rows;j++){
    for(int k = 0;k<mat.columns;k++){
      os << mat(j+1,k+1) << " ";
   }
    os << endl;
 }
  return os;
}

// Main program

int main(){

  // Demonstrate default constructor
  matrix a1;
  cout<<a1;

  // Parameterized constructor
  const int m(2),n(2);
  matrix a2(m,n);
  // Set values for a2 here
  a2[0] = 1; a2[1] = 2; a2[2] = 3; a2[3] = 4;
  // Print matrix a2
  cout<<a2;


  // Deep copy by assignment: define new matrix a3 then copy from a2 to a3
  matrix a3(m,n);
  cout<<a3;
  a3=a2;
  cout<<a3;
  // Modify contents of original matrix and show assigned matrix is unchanged here
  a2[0] = 5;
  cout<<a2;
  cout<<a3; //here is where segmentation fault occurs
  return 0;
}
  • 2
    I suggest fixing the copy constructor and assignment operator first. – juanchopanza Mar 06 '17 at 21:51
  • Then run with a debugger, because your code is full of trivial bugs. – juanchopanza Mar 06 '17 at 21:56
  • 1
    *I've got as far as defining the class* -- You didn't. This matrix class can fall flat on its face with just a two-line main() program. `{ matrix m(1,2); matrix m2=m;}` – PaulMcKenzie Mar 06 '17 at 22:02
  • I cannot reproduce the error (after I add a semicolon needed by the compiler). Is this a minimal example? – Beta Mar 06 '17 at 22:16
  • @user7631642 Already stated you have a big problem in your copy constructor. Do you not see it? You also have a looming bug in your assignment operator. `matrix m; matrix m2(1,2); m = m2;` If I can create havoc with these tiny examples, maybe you should fix those issues first. – PaulMcKenzie Mar 06 '17 at 22:35
  • @PaulMcKenzie Well I'm clearly a beginner, and if I could see it I would have fixed it. Please enlighten me. – user7631642 Mar 06 '17 at 22:47
  • @user7631642 -- In your copy constructor, you allocated no memory for the matrix you are copying to. That should have been obvious, given what you've already written (and this is **not** beginner code). That manifests itself in the little two line program I wrote (why not run it, just to see what may happen?). – PaulMcKenzie Mar 06 '17 at 22:56
  • @PaulMcKenzie I see, got it now. Thanks. – user7631642 Mar 06 '17 at 23:13
  • @user7631642 Also, your assignment operator can easily have been written like this: `{matrix tm(mat); std::swap(tm.mdata,data);std::swap(tm.rows,rows);std::swap(tm.columns,columns);return *this;}`. This avoids the bugs in your assignment operator (you have a few). In addition, don't call `exit(1)` in the middle of class code. [See this](http://stackoverflow.com/questions/22843189/exit-call-inside-a-function-which-should-return-a-reference) – PaulMcKenzie Mar 06 '17 at 23:29

2 Answers2

3

It seems you are going beyond the limits of your matrix. You should remove the +1 for i and j...

for(int j = 0;j<mat.rows;j++){
    for(int k = 0;k<mat.columns;k++){
      os << mat(j,k) << " ";
   }

When an array has 3 elements, this means you can access these 3 elements:

array[0]
array[1]
array[2]

but not array[3] which is the 4th element of your array whose length is 3.

As a rule of thumb, when you get a segmentation fault, you should run your program with gdb or valgrind. These tools will often give you very valuable information to spot the root cause of memory access errors in your code.

SegFault
  • 1,097
  • 1
  • 14
  • 14
  • The length of the array is 4. This is confirmed by the fact that the ostream is used multiple times before the segmentation fault occurs and outputs, as expected, 4 elements. – user7631642 Mar 06 '17 at 22:01
  • I wrote `3` as an example. If the length of your array is 4, then you are trying to access the 5th element (`array[4]`). Segmentation faults are not automatic, ie. your program can run multiple times without crashing, that does not mean you are not doing something dangerous. It depends on many conditions that go beyond the scope of this question. – SegFault Mar 06 '17 at 22:07
0

You need to make the FUNCTION a friend of the class matrix since it cannot access columns or rows as they are are private members. Typical mistake while overloading operator<< and operator>>. Don't ever forget the friend.

jiveturkey
  • 2,484
  • 1
  • 23
  • 41
  • It's actually quite uncommon for operator << to need to be a friend, and in this case it's not, because the class appears to have suitable accessor functions to implement it as a normal, free function. –  Mar 06 '17 at 21:54
  • I missed the declarations, apologies. – jiveturkey Mar 06 '17 at 21:55