1

I just don't see my mistake. There are so many questions regarding this error message and either the answers don't apply or I just can't see that they apply. Maybe the error message should be improved?

Matrix a = Matrix(3, 4);
// fill a with values
Matrix c = Matrix(4, 4);
// fill c with values
a *= c - c; //this is where the compile error occurs

When I change the line to a *= c it works. So I guess there is nothing wrong with the *= operator.

This is the Matrix *= operator:

Matrix &Matrix::operator *=(Matrix &B)
{
    Matrix M(rows(), B.cols());
    for (int i = 0; i<rows(); i++)
    {
        for (int j=0; j<B.cols(); j++)
        {
            for (int k=0; k<cols(); k++)
            {
                M(i,j) = M(i,j) + (*this)(i,k) * B(k,j);
            }
        }
    }
    return M;
}

And this is the -operator:

Matrix operator -(Matrix &A, Matrix &B)
{
    //TODO: Check if matrices have same dimensions, exception else
    Matrix M(A.rows(), A.cols());
    for(int i=0; i<A.rows(); i++)
    {
        for(int j=0; j<A.cols(); j++)
        {
            M(i,j) = A(i,j)-B(i,j);
        }
    }
    return M;
}
Sadık
  • 4,249
  • 7
  • 53
  • 89

2 Answers2

4

By the command c - c you generate a new matrix via operator- and return it. Next, operator*= takes a reference to a matrix, and this is where the compiler complains. It does it in order to prevent you from the fact, that the underlying object will be expired by the time you want to use it.

Try changing the Matrix& to Matrix const&. This will extend the lifetime of your object until the end of the function. Plus, it's is also more appropriate from the const-correctness view.

Moreover, you should return *this from your operator*= and also change the contained matrix. (thanks to @CoryKramer for pointing, missed it in the hurry of answering).

So your operator should basically look like (just the basic concept, no optimization at all):

Matrix &Matrix::operator *=(Matrix const& B)
{
    Matrix M(rows(), B.cols());

    for (int i = 0; i<rows(); i++)
    {
        for (int j=0; j<B.cols(); j++)
        {
            for (int k=0; k<cols(); k++)
            {
                M(i,j) += (*this)(i,k) * B(k,j);
            }
        }
    }

    //copy -- or better move -- the temporary matrix into *this
    operator=(std::move(M));
    return *this;
}
davidhigh
  • 14,652
  • 2
  • 44
  • 75
  • `M` is a local variable. You can't return a reference to it safely. – François Andrieux Apr 12 '17 at 18:39
  • @CoryKramer: of course one should change and return `*this` – davidhigh Apr 12 '17 at 18:47
  • thanks. I thought I'd understand the error now, but with your fix the error still occurs. – Sadık Apr 12 '17 at 18:56
  • @Sadik: I'd set the `operator-` parameters to const-references as well, i.e. `operator-(Matrix const& A, Matrix const& B)`, but this shouldn't be the problem. Is it really the same problem, same error message? (Just asking because the code is explicitly doing what the original error message asked for -- provide a (const) reference to `Matrix const&` ...). Are there other operators involved in your code which are unfixed and might have a similar problem? – davidhigh Apr 12 '17 at 19:02
  • yes, it is exactly the same error message, though it returns `*this` now. Other operators have the same problem, but the compiler is pointing to the same line. – Sadık Apr 12 '17 at 19:20
  • @Sadik: Hard to say where the error is without seeing you code, but I think it should work: see [here](http://coliru.stacked-crooked.com/a/faa9392d37c9793d) for a minimal example (which doesn't do anything a matrix should but correctly captures the types of the function parameters ... without error). – davidhigh Apr 12 '17 at 19:37
  • It's still completely unclear to me why your minimal example works and my code doesn't. My code is not long, maybe I should add a few more lines. – Sadık Apr 12 '17 at 20:28
  • @davidhigh [here is my code](http://coliru.stacked-crooked.com/a/0430bc4142c082e0) – Sadık Apr 12 '17 at 20:39
0

The solution was to change the operator signatures from

Matrix operator -(Matrix &A, Matrix &B);

to

Matrix operator -(const Matrix &A, const Matrix &B)

for all operators. I also had to add a new operator const double & operator ()(int row, int column) const;, otherwise the statement B(k,j) wouldn't work. Additional to that I did what davidhigh suggested and return *this instead of the temporary Matrix object.

The statement where the error occured wasa *= c-c.

At first, c-c will create a temporary object. Then this temporary object is passed by reference to the *= operator. C++ only allows a temporary to be passed to a const reference, value, or rvalue.

Thank you everybody for helping me out.

Sadık
  • 4,249
  • 7
  • 53
  • 89
  • No, prefer `const Matrix&` to just `Matrix` (unless you are going to modify or move the argument). – aschepler Apr 12 '17 at 21:47
  • @aschepler Thank you. I followed your advice and now it's perfectly clean I think. Learned a lot here. – Sadık Apr 12 '17 at 21:57
  • You also need to fix `operator*=` to operate on `*this` and not return a local variable by reference as you currently do (the other answer describes this) – M.M Apr 12 '17 at 21:59
  • I already mentioned this, right? The fix needs to be applied to all operators in the class. Actually the davidhigh mentioned this fix, but said that this was not the real problem. Turned out it was. I'm going to accept his answer. – Sadık Apr 12 '17 at 22:01