0

I'm implementing a matrix class with some arithmetic. This is what I had as function for addition: 

...
Matrix Matrix::operator+(const Matrix& other) const
{
    Matrix out = *this;
    out += scalar;
    return out;
}

void Matrix::operator+=(const Matrix& other)
{
    // Actually adding the numbers. 
}

Just recently I found out that besides the copy constructor, I should also define a move constructor to avoid unnecessary copying of my matrix class. So I did that. And here's where it goes wrong. In the first line of my + operator, it will now move instead of copy and that causes off course a memory error.

So I revised to the following:

...
MatrixNT::MatrixNT(const MatrixNT&& other)
{
    m_logger = LogWrapper::getLogger("MatrixNT");
    m_rows = other.m_rows;
    m_cols = other.m_cols;

    m_data = other.m_data;
}
...

Matrix Matrix::operator+(const Matrix& other) const
{
    Matrix out(*this);
    out += scalar;
    return out;
}

Is this the correct way of calling the copy constructor? It seems a bit odd, using the deference operator again. I can't find any examples on it.

hasdrubal
  • 1,024
  • 14
  • 30
  • 3
    "_In the first line of my + operator, it will now move instead of copy and that causes off course a memory error._" Why do you think, that it will move there? – Algirdas Preidžius May 29 '20 at 09:10
  • 1
    `operator+=` should return `Matrix&`. Do as the basic types do. Anyway, I don't see `*this` as odd. `this` is a pointer due to historical artefacts, so of course one must dereference it to get a reference. There's no other conceivable way to do it, is there? – underscore_d May 29 '20 at 09:15
  • 1
    Why on earth are you defining a move from `const&&`? The point of moving is to save performance, yet you're copying again. And you couldn't move from a const reference even if you told the compiler to do so. Also, is this `Matrix` or `MatrixNT`? – underscore_d May 29 '20 at 09:16
  • According to the docs, it was one way to do so https://en.cppreference.com/w/cpp/language/move_constructor. Pretty much rule of thumb to me whenever I can I usually do. I guess in this case it matter? How? – hasdrubal May 29 '20 at 09:19
  • 1
    The point of moving is to steal resources from the expiring instance. You're copying resources from it, because you don't cast to rvalue reference with `std::move()`. *Also*, even if you did, if the instance was `const&&`, the compiler couldn't move anyway, so it would just copy. You've added a pointless non-moving move constructor. If you don't understand how, or why, to do something, then probably hold off doing it. – underscore_d May 29 '20 at 09:19
  • "_that causes off course a memory error._" What error? Please [edit] to quote that. Have you been able to try debugging to pinpoint/analyse where that error arises? – underscore_d May 29 '20 at 09:24
  • So is this the cause of the issue then? I don't understand that if my move function doesn't move but copy, how I would end up with my memory issue – hasdrubal May 29 '20 at 09:24
  • 1
    If you had shown your definition of the `Matrix` class I'd expect an answer with a working example implementation of the [rule of five](https://en.cppreference.com/w/cpp/language/rule_of_three) had alread been given at this point. – Ted Lyngmo May 29 '20 at 09:28
  • `friend Matrix operator+(Matrix lhs, const Matrix& rhs) const { return lhs += rhs; }` to allow (implicit) move for `m1+m2+m3`. extra overload might be needed as + is symmetric. – Jarod42 May 29 '20 at 09:29
  • @hasdrubal Who knows? You still never explained what that "memory issue" is... nor do we have the full code needed to analyse or reproduce this. – underscore_d May 29 '20 at 09:39
  • 2
    Copy and move are full of pits for beginners. You should read this post on the [copy and swap idiom](https://stackoverflow.com/q/3279543/3545273) and this detailed answer about [move semantics](https://stackoverflow.com/a/11540204/3545273). I could not write better answers... – Serge Ballesta May 29 '20 at 09:47
  • Turned out to be a bug in the move constructor. Didn't release the memory of the m_data there. Assigning it to nullptr solved it – hasdrubal May 29 '20 at 10:37
  • 2
    Great! But if you had shown the full class definition, then someone could have easily identified that and pointed it out to you. – underscore_d May 29 '20 at 11:20

0 Answers0