Segmentation fault when using overloaded operator

Question

The following line in my code is causing a mysterious segfault:

N = N + M;

Where N and M are objects of the Matrix class:

class Matrix {
    vector<int> A; 
    int m, n;
}

+ operator function:

Matrix Matrix::operator+(const Matrix &other){
    //matrices must be of same dimensions
    //if not, return null Matrix
    if (m != other.m || n != other.n)
        return Matrix(0, 0);

    Matrix T(m, n);
    for (int i = 0; i < m*n; i++){
        T.A.at(i) = this->A.at(i) + other.A.at(i);
    }
    return T;
}

Of course N and M are of the same size (3x3).

The segfault appears even with this:

M = N + M;

or

M = M + N;

or

Matrix P;
P = M + N;

but not with:

Matrix P = M + N;

What could possibly be my mistake? I am new to C++.

EDIT: Here's the = operator:

Matrix Matrix::operator=(const Matrix &other){
    A = other.A;
    m = other.m, n = other.n;
}

EDIT 2: My constructor can be helpful

Matrix::Matrix(int r, int c):
    m(r), n(c), A(r*c)
{ }

Are you sure the problem is in the `Matrix::operator+`? Could be in `Matrix::operator=`. Also, `Matrix::operator+` probably should be a `const` member function. — Eljay, Jan 15 '18 at 13:53
Maybe a vector index out of range. Compiling in debug mode and running under a debugger will probably relveal some information. — Jabberwocky, Jan 15 '18 at 13:59
as @Eljay pointed, it seems like problem is in = operator, cause you do not have problem with Matrix P = M + N; (in this case = operator will not be called because of copy elision). And in cases you have problem, = operator will be called — Andrew Kashpur, Jan 15 '18 at 14:04
Segmentation faults in this sort of code are usually easy to diagnose. Make a repeatable test-case; then single step your program observing the values of the variables being used. When you find the bad one workout why. The whole process should take less time then that which you have already invested in posting on SO. — Richard Critten, Jan 15 '18 at 14:11
Your method returns an object - you are replacing that object with another. — falopsy, Jan 15 '18 at 14:20
@AndrewKashpur The assignment operator isn't used in an initialisation not because of copy elision, but because initialisation is not assignment. — molbdnilo, Jan 15 '18 at 14:22
beware, if you do not [edit] your question to provide precision to the queries in the comment, your question will be closed as off-topic. — YSC, Jan 15 '18 at 14:24
Edited the question, i've included the = operator definition — DangerousTim, Jan 15 '18 at 15:00
@molbdnilo yes, and compilers treat this particular case as copy-initialisation instead of default initialisation and assignment because of copy elision optimization. Please correct me if I am wrong. — Andrew Kashpur, Jan 15 '18 at 15:01
@AndrewKashpur Initialisation and assignment are entirely separate things. This is treated as copy-initialisation because that's what it is - it's not an optimisation and it has nothing to do with copy elision. — molbdnilo, Jan 15 '18 at 15:11

GuyRT · Accepted Answer · 2018-01-15T16:04:21.613

I think the problem is with your assignment operator:

Matrix Matrix::operator=(const Matrix &other){
    A = other.A;
    m = other.m, n = other.n;
}

You declare it as returning a Matrix object, but don't actually return anything.

Solution (note the return type is now a reference):

Matrix &Matrix::operator=(const Matrix &other){
    A = other.A;
    m = other.m, n = other.n;
    return *this;
}

Note that the default compiler-generated assignment will do the right thing anyway. So a better solution is to use that one (within the class declaration):

Matrix &operator=(const Matrix &other) = default;

`m = other.m, n = other.n` is _ugly_. (and yeah I know it comes from OP) — YSC, Jan 15 '18 at 21:35

Segmentation fault when using overloaded operator

1 Answers1