0

I need to implement a matrix class for my study, and now I'm stuck with operator + overloading. Here's my matrix.cpp code:

template<class type>
matrix<type> matrix<type>::operator+ (const matrix<type>& matObj){
matrix<type> newMat(this->WIDTH, this->HEIGHT, 0);
    for (int i = 0; i < this->HEIGHT; i++)
        for (int j = 0; j < this->WIDTH; j++)
            newMat.array[WIDTH * i + j] = matObj.array[WIDTH * i + j] + this->array[WIDTH * i + j];

    return newMat;
}

And here's main.cpp code:

int main() {

    vector< vector<int>> v = {
        {1, 2, 3},
        {4, 5, 6},
        {1, 2, 9}
    };
    math::matrix<int> mat1(v), mat2;
    mat2 = mat1+mat1;

    for (int i = 0; i < mat2.cols();  i++) {
        for (int j = 0; j < mat2.rows(); j++) {
            cout << mat2[i][j] << ", ";
        }
        cout << endl;
    }

    cin.get();
    return 0;
}

Assume that all constructors and other codes are written correctly. So the problem is when I try to run the program, it fails on line:

mat2 = mat1+mat1;

And when I try to print newMat in matrix.cpp before the return statement, it prints it correctly. And of course, I have implemented = operator which works fine. Any suggestions?

Amatuer
  • 11
  • 1
  • 1
    There are some handy hints (and a lot of other wisdom) to be found in [What are the basic rules and idioms for operator overloading?](https://stackoverflow.com/questions/4421706/what-are-the-basic-rules-and-idioms-for-operator-overloading) – user4581301 Jan 29 '20 at 20:03
  • 3
    In what way does `mat2 = mat1 + mat1` "fail"? – Nate Eldredge Jan 29 '20 at 20:03
  • Unrelated: You could save yourself a bit of work and have only one loop that goes to `HEIGHT * WIDTH`. `newMat.array[WIDTH * i + j] = matObj.array[WIDTH * i + j] + this->array[WIDTH * i + j];` becomes `newMat.array[i] = matObj.array[i] + this->array[i];` Less room for typos, too. – user4581301 Jan 29 '20 at 20:07
  • 1
    What does `array` look like? Have you taken the [Rule of Three(and friends)](https://en.cppreference.com/w/cpp/language/rule_of_three) into account? Build yourself a [mcve] and if that doesn't help you find the problem, add the [mcve] to the question. – user4581301 Jan 29 '20 at 20:09
  • 2
    It seems like the `array` object is capable of dynamic allocation (a `std::vector` perhaps). If so, shouldn't you check that the dimensions of the added `matrix` has the same size as the one you are adding to? Perhaps it'd be better to make the dimensions part of the type? – Ted Lyngmo Jan 29 '20 at 20:14
  • If you implement that as a method it should be `const` in this case, but even better it should be a standalone (possibly friend) function. – Slava Jan 29 '20 at 20:34
  • 1
    *"Assume that all constructors and other codes are written correctly."* Well, we shouldn't. How have you implemented the default constructor, the constructor that takes a `std::vector>` and the copy assignment? Also, in the first posted snippets, the nested loop is row-major (and so appear to be stored data), while the constructor accepts the dimensions as **(columns, rows)** and, in the other snippet, the nested loops are column-major. In your example, the matrix is square, so it doesn't make any difference, but the code should be consistent. – Bob__ Jan 30 '20 at 09:56

1 Answers1

0

The easy way to implement + is as follows.

struct example {
  int state = 0;

  example& operator+=( example const& rhs )& {
    // increment state here; change in real code
    state += rhs.state;
    return *this;
  }
  friend example operator+( example lhs, example const& rhs ) {
    lhs += rhs;
    return std::move(lhs);
  }
  example( example&& ) = default; // ensure this is efficient
  example& operator=( example&& ) = default; // ensure this is efficient
  example& operator=( example const& ) = default; // this can be expensive
  example( example const& ) = default; // this can be expensive
};

more concretely

template<class T>
struct matrix {
  std::vector< std::vector<T> > state;

  matrix & operator+=( matrix const& rhs )& {
    // increment state here; change in real code
    for (std::size_t i = 0; i < (std::min)(state.size(), rhs.state.size(); ++i) {
      for (std::size_t j = 0; j < (std::min)(state[i].size(), rhs.state[i].size(); ++j) {
         state[i][j] += rhs.state[i][j];
      }
    }
    return *this;
  }
  friend matrix operator+( matrix lhs, matrix const& rhs ) {
    lhs += rhs;
    return std::move(lhs);
  }
  matrix( matrix&& ) = default; // ensure this is efficient
  matrix& operator=( matrix&& ) = default; // ensure this is efficient
  matrix& operator=( matrix const& ) = default; // this can be expensive
  matrix( matrix const& ) = default; // this can be expensive
}

=default works on matrix(matrix&&) etc because I followed the rule of 0 -- I left data storage to a specialized class (std::vector<?>), and it has efficient move semantics, so I get it automatically.

If you swap state for a std::vector< T > that has width*height elements, you just have to adapt how you access it. (That is more efficient, but I was trying to keep it simple).

Yakk - Adam Nevraumont
  • 235,777
  • 25
  • 285
  • 465
  • 3
    Doesn't `return std::move(lhs);` prevent NRVO, in this case? – Bob__ Jan 29 '20 at 20:29
  • It is NOT recommended to use `std::move` on returns – Slava Jan 29 '20 at 20:32
  • Implementing plus operator via plus equal operator adds a needless copy and should be avoided. – ezegoing Jan 29 '20 at 20:46
  • @Bob__ No, it is impossible to NRVO a function argument. The `std::move` is there to remind the programmer of that fact. – Yakk - Adam Nevraumont Jan 29 '20 at 20:51
  • @Slava It is impossible to elide function arguments. – Yakk - Adam Nevraumont Jan 29 '20 at 20:51
  • @ezegoing There are no additional "copies", there is additional moves. Presuming cheap move, this is ridiculously optimal. Examine `matrixR = matrixA + matrixB + matrixC` -- only one `matrix` internal state is created, then it is moved a bunch and incremented. – Yakk - Adam Nevraumont Jan 29 '20 at 20:52
  • There is an additional copy and no modern compilers wont do anything about it! http://quick-bench.com/7UWu8Qf0datzb-NXczfYbwBl6h4 – ezegoing Jan 29 '20 at 20:53
  • Actually according to this https://stackoverflow.com/a/14857144/432358 you should not use `std::move() on returning parameter passed by value, but rather rvalue reference. Is it not your answer? – Slava Jan 29 '20 at 21:03
  • @Slava That answer is from many years ago. The second example could be improved by using `return std::move(v)`, to avoid making a copy of the vector (which may have been omitted because the original version did not benefit from it). – 1201ProgramAlarm Jan 29 '20 at 21:41
  • @1201ProgramAlarm something changed in standard since C++11 so that answer is not actual anymore? Should it be updated? – Slava Jan 29 '20 at 21:59
  • @ezeg Looking at the assembly, that is because a=0, a=b+c appears faster than a=b, a+=c, in terms of SIMD instructions (and the optimizer doesn't solve that they are the same). I do not see an additional allocation in the assembly. Did I miss it? – Yakk - Adam Nevraumont Jan 29 '20 at 22:17
  • @Slava I guess that this part of that answer *"If you have a local variable (**be it a parameter or not**), returning it **implicitly** moves (and this implicit move **can be elided away**, while an explicit move cannot)."* should be rephrased, given [this](https://eel.is/c++draft/class.copy.elision#def:entity,implicitly_movable) or the English [version](https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c/) ([here](http://vmpstr.blogspot.com/2015/12/redundant-stdmove.html) too). If I understand correctly, it can't be elided and the `std::move` is redundant. – Bob__ Jan 29 '20 at 23:52
  • @Yakk-AdamNevraumont I did not look at assembly. What I was referring to was the copy you introduce by passing by value. Instead you can simply pass by reference and allocate space for a result vector and then perform the plus operation. Even if there is an initialization during the process of allocation space for the vector, it is going to be against a constant. Considering your thoughts in your comment and expanding them by 1 dimension and say a,b are vectors now then a=0 is cheaper than a=b. – ezegoing Jan 30 '20 at 01:21
  • @ezegoing The copy (from the argument) is the same as creating the local. Except the local is zero initialized (which can be optimized to free sometimes!), and the argument is a copy of one of the two arguments. Except when you do `a+b+c+d`, the return value of `a+b` is elided into the `(a+b)+c` call, which is elided into `((a+b)+c)+d`. Doing this with `const&` and `&&` arguments requires two nearly identical overloads. `a=0` actually is resizing the entire vector to be full of 0s in your code. – Yakk - Adam Nevraumont Jan 30 '20 at 02:43
  • @ezegoing See http://quick-bench.com/k5jLoCF3aEAm7Nq4deu2jT4Fy-8 -- chained `+` gets faster, especially with larger arrays. Strange that no time is spent allocating memory however. – Yakk - Adam Nevraumont Jan 30 '20 at 02:59
  • @Yakk-AdamNevraumont please take a look at my code in GitHub [link](https://github.com/MarkV3/ME), my + operator still doesn't work, maybe you'll find a way to fix it – Amatuer Jan 31 '20 at 08:36
  • @Amatuer (a) it doesn't look like you followed my advice above (I don't see a friend operator+ for example, and you are doing manual memory management, and your `=` isn't `=default`) so I'm not surprised your code has bugs, and (b) "doesn't work" isn't a useful description of a problem, and (c) if you have a question, the [ask question] button together with a [mcve] is useful and (d) COLS, ROWS, WIDTH and HEIGHT?! You are doing something very strange for a matrix to have all 4. – Yakk - Adam Nevraumont Feb 03 '20 at 18:21