c++ empty copy assignment operator copies data

Question

I am sort of new to c++ and wanted to write a small matrix library which has a base class Matrix from which some new matrix types like SparseMatrix, DenseMatrix, HashMatrix etc.

My base class looks like this:


class Matrix {


protected:
    int m,n;


public:
    
    
    Matrix(int m_p, int n_p) : m(m_p), n(n_p){
        std::cout << "gen constr" << std::endl;
    }

    Matrix(Matrix& other) = delete;

    Matrix() = delete;
    
    virtual ~Matrix() {}


    int getM() const {
        return m;
    }

    int getN() const {
        return n;
    }

    virtual double& get(int m, int n) = 0;

    virtual double  get(int m, int n) const = 0;

    inline double& operator() (int m, int n){
        return get(m,n);
    }

    inline double  operator() (int m, int n) const{
        return get(m,n);
    }

    friend std::ostream &operator<<(std::ostream &os, Matrix &matrix) {

        using std::scientific;
        using std::fixed;

        os << std::fixed << std::setprecision(2) << scientific << std::setfill(' ');

        for(int i = 1; i <= matrix.getM(); i++){
            for(int j = 1; j <= matrix.getN(); j++){
                os << std::setw(10) << matrix.get(i,j)  << " ";
            }
            os << "\n";
        }
        return os;
    }
    
    
    Matrix& operator=(const Matrix& other) {
//        std::cout << "equality assign" << std::endl;
        return *this;
    }

};

As you can see, I have overwritten the equality assign operator which simply returns the object and does not actually copy values.

My first implementation of DenseMatrix is very straight forward:

class DenseMatrix : public Matrix{


private:
    double* data;

public:

 
    DenseMatrix(int mP, int nP) : Matrix(mP, nP){
        std::cout << "gen constr base" << std::endl;
        this->data = new double[mP * nP]{0};
    }
        
    
    
    
    DenseMatrix() = delete;

    ~DenseMatrix() {
        delete this->data ;
    }

    double &get(int m, int n) {
        int index = m*getN()+n-(getN()+1);
        assert(index < (getN() * getM()) && index >= 0);
        return this->data [index];
    }

    double get(int m, int n)const {
        int index = m*getN()+n-(getN()+1);
        assert(index < (getN() * getM()) && index >= 0);
        return this->data [index];
    }
 
      
};

Furthermore the main() function looks like this:

    DenseMatrix mat(3,3);
    for(int i = 1; i<= 3; i++){
        mat(i,i) = i;
    }
    
    DenseMatrix mat2(3,3);
    mat2 = mat;
    
    
    std::cout << mat << std::endl;
    std::cout << mat2 << std::endl;


>>>  1.00e+00   0.00e+00   0.00e+00 
>>>  0.00e+00   2.00e+00   0.00e+00 
>>>  0.00e+00   0.00e+00   3.00e+00 

>>>  1.00e+00   0.00e+00   0.00e+00 
>>>  0.00e+00   2.00e+00   0.00e+00 
>>>  0.00e+00   0.00e+00   3.00e+00

As you can see, I create two matrices first. I adjust the values for the first matrix and leave the values of the second matrix default to 0. Yet after calling the equality assign operator, the content of the second matrix changes even tho the function I implemented has basically no code which affects the matrix.

I do not understand this behavior and would be very happy if someone could briefly explain what is going on here.

Thank you very much for your patience and help :)

Handy reading: [The rule of three/five/zero](https://en.cppreference.com/w/cpp/language/rule_of_three). — user4581301, Jul 28 '20 at 21:06
*The content of the second matrix changes even tho the function I implemented has basically no code which affects the matrix.* -- I see no implementation for the `DenseMatrix` assignment operator. Second, even if you meant for the `Matrix` class `operator=` to be called, it would have given you erroneous results, since you would have a double `delete` error on destruction. One thing you have to realize is that you're either all-in or all-out if you are deciding to make a class have correct copy semantics. Either turn off all the copy functions, or implement them fully. — PaulMcKenzie, Jul 28 '20 at 21:08
Just to make sure; `virtual Matrix& operator=(const Matrix& other) = 0;` In the `Matrix` will force my sub classes to override that function and I should be fine? — Finn Eggers, Jul 28 '20 at 21:15
That trick doesn't work as well as you'd like. [Consider making a virtual `clone` function](https://stackoverflow.com/questions/12902751/how-to-clone-object-in-c-or-is-there-another-solution) instead. — user4581301, Jul 28 '20 at 21:21
@FinnEggers Better solution is to use a smart pointer. Then you only need to implement the copy operations. And they will be easier to make exception safe (although given that elements are integers, exceptions are probably not an issue). — eerorika, Jul 28 '20 at 21:46

score 2 · Accepted Answer · answered Jul 28 '20 at 21:04

2

You have not deleted or defined the operator= copy-assignment operator for DenseMatrix, so the compiler will synthesize one for you. This function is going to do a member wise copy of the data members, in this case double *data. Since the pointer is going to point to the same contents in both mat and mat2, you see the same contents when you print them out.

Note that this is probably not what you want. One issue is that your DenseMatrix destructor is going to delete data twice, leading to possibly a segfault.

answered Jul 28 '20 at 21:04

cigien

57,834
11
73
112

Oh alright. I guess C++ is a little bit more tricky than Java when it comes to this. Thank you very much for that precise answer :) About the destructor, does it make sense in general to check if its a nullptr and if its not, delete it and assign a nullptr? – Finn Eggers Jul 28 '20 at 21:06
No, assuming you follow the rule of 3/5/0, then you can unconditionally `delete` `data` (if it's `nullptr` it's a no-op). Also, you don't need to assign `nullptr` after deleting since you won't be looking at `data` after the destructor runs. – cigien Jul 28 '20 at 21:08
Guess I will need to strictly follow that rule. But the copy constructor would be a problem. Because I would create a function: copyDataTo(Matrix& other) which copies the contents of a matrix to another one. In the copy constructor I would call that on the given class but that method requires the set/get methods to be implemented. Allthough they are virtual and during the call in the copy constructor not defined in the new object – Finn Eggers Jul 28 '20 at 21:09
I'm not entirely sure what you mean. You can allocate new memory for `data` and copy the contents from the argument's `data`. – cigien Jul 28 '20 at 21:11
You definitely need to learn 3/5/0. You can't write effective, complex C++ programs without a good grasp of it. You'll also want to make sure you are familiar with [the concept of RAII](https://stackoverflow.com/questions/2321511/what-is-meant-by-resource-acquisition-is-initialization-raii) because it opens up resource management possibilities that are totally different from those available in Java. – user4581301 Jul 28 '20 at 21:12
@user4581301 Thats exactly what this project is supposed to do. – Finn Eggers Jul 28 '20 at 21:12
@cigien Let's assume I have a sparse matrix and would want to copy the data to another matrix, i wont know the type of the other matrix. So I would use the set method. But will I be able to call that during the copy constructor? – Finn Eggers Jul 28 '20 at 21:14
1

Suggestion: Don't make a copy function. [Make a good copy constructor, and then use the copy constructor as the basis for other copying](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom), the assignment operator for example. Note that this isn't always the fastest way to make copies, but it's really easy to write and very hard to get wrong. I start with Copy and Swap and use something harder only when profiling the program says I need to. Doesn't happen very often. – user4581301 Jul 28 '20 at 21:18
Yeah I might try that first. I also added `DenseMatrix &operator=(const Matrix &other) { std::cout << "copy assign" << std::endl; return *this; }` But its not working... – Finn Eggers Jul 28 '20 at 21:19
1

That appears to be a separate question. If so, then try solving that, and then post a new question if you run into issues. Also, consider accepting this answer if it answers your question. – cigien Jul 28 '20 at 21:21

score 2 · Answer 2 · answered Jul 28 '20 at 21:04

You didn't define assignment operator for DenseMatrix.

The implicitly generated assignment operator assigns the sub objects. The base sub object will be assigned using the overloaded assignment operator of Matrix and therefore the dimensions stored within the base are not modified - but you had specified the dimensions to be the same, so they would have been assigned with the same values anyway.

c++ empty copy assignment operator copies data

2 Answers2