-1

Say I have a (immutable) matrix class that dynamically creates an array in the constructor, and deletes it in the deconstructor.

template <typename T>
class matrix {
private:
    T* data;
public:
    size_t const rows, cols;
    matrix(size_t rows, size_t cols) : rows(rows), cols(cols) {
        data = new T[rows*cols];
    }
    ~matrix() {
        delete [] data;
    }
    //access data
    T& operator()(size_t row, size_t col) {
        return data[row*cols + col];
    }
    matrix<T>& operator=(const matrix<T>& other) {
        //what will this->data contain? do I need to delete anything here?
        //should I call the constructor?
        rows = other.rows;
        cols = other.cols;
        data = new T[rows*cols];
        std::copy(&data[0],&data[0] + (sizeof(T)*rows*cols),&other.data[0]);
        return *this;
    }
}

Because I don't have a default constructor within the operator= function, the data in this is just garbage, right? Even if I had a default constructor would it be called? I'm basing the above code off of the example here. Note that I have left out input validation/bounds checking for brevity.

EDIT: I would like to clarify that I'm only concerned for a call like this:

matrix<int> A = B;

Where B has already been initialized.

McAngus
  • 1,826
  • 18
  • 34
  • 6
    You should be more concerned about the memory leak. – juanchopanza Mar 08 '17 at 23:12
  • You forgot the user-defined copy constructor for `matrix`. If you wrote that first, then the assignment operator is very simple., – PaulMcKenzie Mar 08 '17 at 23:15
  • Possible duplicate: http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three – πάντα ῥεῖ Mar 08 '17 at 23:23
  • Huh. `this->data` will be what it was before you called `operator=`. – M.M Mar 08 '17 at 23:23
  • 2
    Your latest edit shows a call to the copy constructor, *not* assignment operator. So my previous comment stands -- you need to write a copy constructor. – PaulMcKenzie Mar 08 '17 at 23:28
  • 3
    The original question is asked under the misconception that the assignment operator is involved. Given the latest edit, it isn't involved at all. Closing as "unclear". – PaulMcKenzie Mar 08 '17 at 23:32
  • 1
    In addition, if you wrote a copy constructor (and it is correct), then the example you linked to that uses `std::swap` in the assignment operator would be, more or less, how you would have written your assignment operator, i.e. using simple calls to `swap` all the members. – PaulMcKenzie Mar 08 '17 at 23:38
  • 1
    `matrix A = B;` is *NOT* an assignment, it's an initialization. So it does not use or call `operator=` at all -- it calls the copy constructor. – Chris Dodd Mar 09 '17 at 01:55

4 Answers4

5

If you use std::vector to store your data, your class becomes a lot simpler

template <typename T>
class matrix {
   std::vector<T> data;
public:
    size_t const rows, cols;
    matrix(size_t rows, size_t cols) : rows(rows), cols(cols) {
        data.resize(rows*cols);
    }
    //access data
    T& operator()(size_t row, size_t col) {
        return data[row*cols + col];
    }
}

you now no longer have to worry about leaking memory, and you don't need to write a destructor, copy constructor or assignment operator.

Ðаn
  • 10,934
  • 11
  • 59
  • 95
3

Say I have a (immutable) matrix class that dynamically creates an array in the constructor, and deletes it in the deconstructor.

You are violating the Rule of Three by not implementing a copy constructor (and the Rule of Five in C++11 by not implementing a move constructor and a move assignment operator).

Your copy assignment operator has a memory leak, as it is not delete[]'ing the old data array before assigning the new[]'ed array.

Try this instead:

template <typename T>
class matrix {
private:
    T* data;
    size_t const rows, cols;

public:
    matrix(size_t rows, size_t cols) : rows(rows), cols(cols) {
        data = new T[rows*cols];
    }

    matrix(const matrix<T> &src) : rows(src.rows), cols(src.cols) {
        data = new T[rows*cols];
        std::copy(data, &data[rows*cols], src.data);
    }

    /* for C++11:

    matrix(matrix<T> &&src) : rows(0), cols(0), data(nullptr) {
        std::swap(rows, src.rows);
        std::swap(cols, src.cols);
        std::swap(data, src.data);
    }
    */

    ~matrix() {
        delete [] data;
    }

    T& operator()(size_t row, size_t col) {
        return data[(row * cols) + col];
    }

    T operator()(size_t row, size_t col) const {
        return data[(row * cols) + col];
    }

    matrix<T>& operator=(const matrix<T>& other) {
        if (&other != this) {
            delete[] data;
            rows = other.rows;
            cols = other.cols;
            data = new T[rows*cols];
            std::copy(data, &data[rows*cols], other.data);
        }
        return *this;
    }

    /* for C++11:

    matrix<T>& operator=(matrix<T> &&other) {
        delete[] data;
        data = nullptr;
        rows = cols = 0;

        std::swap(rows, other.rows);
        std::swap(cols, other.cols);
        std::swap(data, other.data);

        return *this;
    }
    */
};

However, the copy-and-swap idiom would be safer:

template <typename T>
class matrix {
private:
    T* data;
    size_t const rows, cols;

public:
    matrix(size_t rows, size_t cols) : rows(rows), cols(cols) {
        data = new T[rows*cols];
    }

    matrix(const matrix<T> &src) : rows(src.rows), cols(src.cols) {
        data = new T[rows*cols];
        std::copy(data, &data[rows*cols], src.data);
    }

    /* for C++11:

    matrix(matrix<T> &&src) : rows(0), cols(0), data(nullptr) {
        src.swap(*this);
    }
    */

    ~matrix() {
        delete [] data;
    }

    T& operator()(size_t row, size_t col) {
        return data[(row * cols) + col];
    }

    T operator()(size_t row, size_t col) const {
        return data[(row * cols) + col];
    }

    void swap(matrix<T>& other) noexcept
    {
        std::swap(rows, other.rows);
        std::swap(cols, other.cols);
        std::swap(data, other.data);
    }

    matrix<T>& operator=(const matrix<T>& other) {
        if (&other != this) {
            matrix<T>(other).swap(*this);
        }
        return *this;
    }

    /* for C++11:

    matrix<T>& operator=(matrix<T> &&other) {
        other.swap(*this);
        return *this;
    }
    */
};

In the latter case, the copy assignment and move assignment operators can be merged together into a single operator in C++11:

matrix<T>& operator=(matrix<T> other) {
    other.swap(*this);
    return *this;
}

Or, you could just follow the Rule of Zero by using std::vector and let the compiler and STL do all of the work for you:

template <typename T>
class matrix {
private:
    std::vector<T> data;
    size_t const rows, cols;

public:
    matrix(size_t rows, size_t cols) : rows(rows), cols(cols), data(rows*cols) {
    }

    T& operator()(size_t row, size_t col) {
        return data[(row * cols) + col];
    }

    T operator()(size_t row, size_t col) const {
        return data[(row * cols) + col];
    }
};

Because I don't have a default constructor within the operator= function, the data in this is just garbage, right?

No, because operator= can only be called on a previously constructed object, just like any other class instance method.

Even if I had a default constructor would it be called?

In the example you showed, no.

I would like to clarify that I'm only concerned for a call like this:

matrix<int> A = B;

That statement does not call operator= at all. The use of = is just syntax sugar, the compiler actually performs copy construction as if you had written this instead:

matrix<int> A(B);

Which requires a copy constructor, which you have not implemented, and the compiler-generated copy constructor is not sufficient to make a deep copy of your array.

Copy assignment would look more like this instead:

matrix<int> A; // <-- default construction
A = B; // <-- copy assignment

matrix<int> A(B); // <-- copy construction
A = C; // <-- copy assignment
Community
  • 1
  • 1
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • So in the Rule of Zero example `rows`, `cols` and `data` are taken care of by the default copy constructor? – McAngus Mar 09 '17 at 02:07
  • @McAngus: yes, the compiler-generated copy constructor and copy assignment operator are both adequate for the `rows` and `cols` members, and for the `data` member when it is a Rule-of-Three/Five/Zero compliant type like `std::vector`. – Remy Lebeau Mar 09 '17 at 03:04
2

Because I don't have a default constructor within the operator= function, the data in this is just garbage, right?

No.

Even if I had a default constructor would it be called?

No


However, there is a memory leak. You are not deallocating the memory that was allocated when the object was constructed.

mike
  • 4,929
  • 4
  • 40
  • 80
R Sahu
  • 204,454
  • 14
  • 159
  • 270
-1

You might initialize a matrix object and then use operator= for this object. In this case, data in this matrix object would not be garbage because it is already initialized.

If you are using operator= for an uninitialized instance of matrix, e.g matrix<int> a = b;, where b is already initialized, that means you are calling the copy constructor, which is automatically generated by the compiler. In both cases, there are no garbage values.

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
Enes Keles
  • 141
  • 1
  • 15