Segmentation fault when creating a row-major array

Question

I'm trying to implement a row-major array, which is basically a single dimension representation of a 2D array.

This is my class definition

class RMA{
    public:

    RMA(){
        size_=0;
        row_=0;
        column_=0;
        arr_ = new double[size_];
    }

    RMA(int n, int m){
        size_ = n*m;
        column_ = m;
        row_ = n;
        if(size_== 0) arr_ = 0;
            else arr_ = new double[size_];
    }

    RMA(const RMA& arr) {
        size_ = arr.size_;
        if(this != &arr){
            delete [] arr_;
            arr_ = new double[size_];
            for(int i=0; i<size_; i++){
                arr_[i] = arr.arr_[i];
            }
        }
        return *this;
    }

    const double& operator() (int n, int m) const{
        return arr_[n*column_+m];
    }

    double& operator()(int n, int m){
        return arr_[n*column_+m];
    }  
    ~RMA(){delete[] arr_ ;}

    private:
        int size_;
        int column_;
        int row_;
        double* arr_;
}

I've a calling function which creates the array.

RMA create_array() {
    RMA arr;
    arr = RMA(N, M);
    std::cout<<"success";
    return arr;
}

And this is my client

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

    RMA arr = create_array();
    return 0;
}

I end up getting segmentation fault. What am I doing wrong.

Where does the fault occur? What were you attempting to do when it occurred? — Scott Hunter, Feb 17 '15 at 21:51
`arr_ = new double[size_];` how big was that `size_` again ? And your code violates at least one part of the [**`Rule of Three`**](http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)) *multiple* times. — WhozCraig, Feb 17 '15 at 21:55
You violated the [rule of three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). — Alan Stokes, Feb 17 '15 at 22:06

Valeri Atamaniouk · Answer 1 · 2015-02-17T22:19:17.313

1

You use operations, that instead of cloning array, take a shallow copy of an object, and when destructors are used, they try to release the same memory block.

Implement the following operations:

RMA::RMA(const RMA&); // copy constructor - clone buffer
RMA& operator=(const &RMA); // assignment - clone buffer, release old

Also instead of:

 RMA rma;
 rma = RMA(a,b);

Use:

 RMA rma = RMA(a,b) or RMA rma(a,b);

Edit: constructor code:

RMA::RMA(const RMA &rma) : size_(0), row_(0), column_(0), buffer_(0)
{
   *this = rma;
}

RMA &operator=(const RMA&rma)
{
   double *old = buffer_;
   size_ = rma.size_;
   row_ = rma.row_;
   column_ = rma.column_;
   buffer_ = new double[size_];
   memcpy(buffer_, rma.buffer_, sizeof(buffer_[0]) * size_);
   delete []old;
   return *this;
}

edited Feb 17 '15 at 22:19

answered Feb 17 '15 at 22:03

Valeri Atamaniouk

5,125
2
16
18

Added the copy constructor, but now I'm getting the error returning a value from a constructor return *this; – Feb 17 '15 at 22:12
@Antithesis constructor shall not return value. – Valeri Atamaniouk Feb 17 '15 at 22:20
Do i need to add double buffer_ to my private attributes.Not sure why you added this. – Feb 17 '15 at 22:33
That copy-ctor is exactly the *opposite* of a [copy/swap idiom](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom), which would be preferred. If you're going provide one defined via invoke of another regarding copy-ctor and assignment operator, this is exactly the opposite way it should be done. – WhozCraig Feb 17 '15 at 23:33
You should throw in the move ctor & assignment operator too. – Quentin Feb 18 '15 at 03:31

score 0 · Answer 2 · answered Feb 18 '15 at 04:59

The best solution is to get rid of all the new/delete, copy-constructors, and fluff. Use a private member variable to manage the memory, and follow the Rule of Zero. Like this:

struct RMA
{
    RMA(size_t r = 0, size_t c = 0)
        : row(r), column(c), arr(r * c) {}

    const double& operator() (int n, int m) const
        { return arr[n * column + m]; }

    double& operator() (int n, int m)
        { return arr[n * column + m]; }

private:
    std::vector<double> arr;
    size_t row, column;
};

That's it. You should not write any copy-constructor, assignment operator, move whatever, because the default-generated ones already do the right thing.

NB. row is actually redundant in my example too, you could remove it and calculate it when needed as arr.size() / column.

You could use .at( ) instead of [ ] on vector in order to throw an exception for out-of-bounds access, instead of causing undefined behaviour.

Segmentation fault when creating a row-major array

2 Answers2