Segmentation fault but array object is not going out of bounds (C++)

Question

There are other posts about common causes of segmentation faults, but I don't think the built-in array object I've created here (result) doesn't go out of bounds when I assign values to it.
I think this could be helpful to people in the future who have arrays not out of bounds, and I also haven't seen a lot of stuff about making 2D built-in array objects - examples I've seen are almost entirely vectors or std:array objects.

Here is runnable, relevant code:

matrix.h

#ifndef MATRIX_H
#define MATRIX_H

#include <initializer_list>
using std::initializer_list;

typedef unsigned int uint;

class Matrix {

  public:

    Matrix(uint rows, uint cols);  
    ~Matrix();  
    Matrix add(double s) const;  
    const uint numRows() const;  
    const uint numCols() const;  
    double & at(uint row, uint col);  
    const double & at(uint row, uint col) const;  

  private:

    uint rows, cols;
    double ** matrix;

    void makeArray() {
      matrix = new double * [rows];
      for(uint i = 0; i < rows; ++i) {
        matrix[i] = new double [cols];
      }
    }

};

#endif

matrix.cpp

#include "matrix.h"
Matrix::Matrix(uint rows, uint cols) {
  //Make matrix of desired size
  this->rows = rows;
  this->cols = cols;

  makeArray();

  //Initialize all elements to 0
  for(uint i = 0; i < rows; ++i) {
    for(uint j = 0; j < cols; ++j) {
      this->matrix[i][j] = 0.0;
    }
  }
}
Matrix::~Matrix() {
  for(uint i = 0; i < numRows(); ++i) {
    delete[] matrix[i];
  }
  delete[] matrix;
}
const uint Matrix::numRows() const {
  return this->rows;
}

const uint Matrix::numCols() const {
  return this->cols;
}

double & Matrix::at(uint row, uint col) {
  return matrix[row][col];
}

const double & Matrix::at(uint row, uint col) const {
  return matrix[row][col];
}

Matrix Matrix::add(double s) const {
  uint r = this->numRows();
  uint c = this->numCols();

  Matrix * result;
  result = new Matrix(r, c);

  for(uint i = 0; i < r; ++i) {
    for(uint j = 0; j < c; ++j) {
      result->at(i,j) = (this->at(i,j)) + s;
    }
  }

  return * result;
}

main.cpp

#include <iostream>
#include <cstdlib>
#include "matrix.h"

using namespace std;

typedef unsigned int uint;

int main() {

  Matrix * matrix;
  matrix = new Matrix(3, 2); //Works fine

  double scaler = 5;

  matrix->at(2,1) = 5.0; //Works fine

  Matrix r = matrix->add(scaler); //DOESN'T WORK

  return EXIT_SUCCESS;
}

Any ideas why the add function is causing a segmentation fault error? The for-loop I used to fill the result Matrix object doesn't go out of bounds, and I'm not familiar enough with C++ to know what else could be causing it.
Thanks in advance.

There are probably other issues. Don't try to manage dynamic memory allocation yourself, unless you're absolutely sure you need to. — πάντα ῥεῖ
– πάντα ῥεῖ, Commented Jul 10, 2016 at 19:44
Should we ignore the memory leak in your add() member, and just concentrate on a much bigger issue, namely What is the Rule of 3 ? — WhozCraig
– WhozCraig, Commented Jul 10, 2016 at 19:44
Matrix * result; [...] return * result; why do you do this? Btw I dont see any good reason for a single * in your code — 463035818_is_not_an_ai
– 463035818_is_not_an_ai, Commented Jul 10, 2016 at 19:51
Unrelated to the error, but a general suggestion: since you don't want to represent a jagged matrix, and the dimensions are fixed at construction, it may be better to store the data in one single contiguous array. You could further simplify the code by using an std::vector<double> as data storage. — juanchopanza
– juanchopanza, Commented Jul 10, 2016 at 19:57
@Mazzone -- You're returning a Matrix by value in your add function and you failed to implement a user-defined copy constructor or assignment operator. You don't even have stubs of these functions, to indicate that you had knowledge of them.. You say you have to write things this way, but were you not told about these basic things, like the "rule of 3"? Why are you being told to do things a certain way, and you're not told all of the preliminaries required to accomplish the goal? — PaulMcKenzie
– PaulMcKenzie, Commented Jul 10, 2016 at 20:37

Peter · Accepted Answer · 2016-07-10 20:11:46Z

The problem is lack of a manually defined copy constructor or assignment operator, given that the class manages a resource (memory).

If an instance of the class is assigned, or used to create a copy, the result will be two distinct objects that reference the same memory. When those two objects which refer to the same memory are destroyed, the memory is released twice. The result of that is undefined behaviour.

Look up the "rule of three" for a solution. In C++11, that often becomes a "rule of five" or a "rule of zero" (which involves using techniques to avoid the problem in the first place).

There is also a pretty significant problem in the add() function, since it dynamically creates a Matrix, then returns a copy of it. That causes a memory leak, even if the problem with copying the object is resolved. That function actually looks like something which would be written in a garbage collected language - the problem being that C++ is not garbage collected.

Collectives™ on Stack Overflow

Segmentation fault but array object is not going out of bounds (C++)

1 Answer 1

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related