Deep copy in a move constructor

Question

I am new to C++11 so i still strugle with its concepts.

Here's my problem :

I have a matrix class :

class matrix
{

private:

  double** data;
  size_t number_lines;
  size_t number_columns;

  size_t capacity_lines;
  size_t capacity_columns;

public:
....

}

and i've provided a copy constructor, a move constructor...

I've overloaded the multiplication operator *(double x) to multiply matrix elements by the scalar x and return the multiplied matrix. Here's the code :

matrix matrix::operator*(double lambda)
{
double** aux_data = new double*[number_lines];
for (size_t i = 0; i < number_lines; i++)
{
    aux_data[i] = new double[number_columns];
    for (size_t j = 0; j < number_columns; j++)
        aux_data[i][j] = lambda*data[i][j];
}
return matrix(aux_data, number_lines, number_columns);
}

the return of the function is an rvalue reference so it invokes the move constructor. Here's the code of the move constructor :

matrix::matrix(const matrix&& moved_copy)
{
if (this != &moved_copy) 
{
    number_columns = moved_copy.number_columns;
    number_lines = moved_copy.number_lines;
    data = moved_copy.data;
}
}

The problem with this move constructor is that it performs a shallow copy and not a deep copy (like every move constructor i guess, otherwise what's the point of this move constructor) so the member data points to the object pointed by moved_copy.data, but this object is local to the operator *=() function so when the operator goes out of scope the object is gone and i have a dangling pointer. So my question is : should i perform a deep copy in the move constructor or is there way of solving this problem without doing so ?

Thank you.

what `operator*=` ? Your comments make no sense. There is no dangling pointer. But you need to reset the members of `moved_copy` , a move constructor should move and not copy. Move constructors move, they don't copy (neither shallow nor deep). — M.M, Mar 22 '17 at 10:51
You need to make the moved-from object safe to destroy. How you do that is up to you. Normally setting its poinrers to nullptr should be enough. You however should not use any pointers directly unless you really have to. Use std::vector for all your array-related needs and never worry about copying, moving or destroying your arrays again. — n. m. could be an AI, Mar 22 '17 at 10:53
@M.M : when i'm gonna assign the return of the operator to a matrix object, this object will have a dangling pointer member. — M.Azzeddine, Mar 22 '17 at 11:04
@n.m. : you're right by setting the pointer to nullptr was enough thank you. Normally i would use a vector class but i want the code to run faster so i chose to directly use pointers. — M.Azzeddine, Mar 22 '17 at 11:07
Using a vector *is* using pointers, just with a load of convenience functions. Everything will be inlined to efficient pointer arithmetic. — Joseph Ireland, Mar 22 '17 at 11:11

Miles Budnek · Answer 1 · 2017-03-22T11:16:30.413

6

No, you shouldn't make a deep copy in a move constructor. The whole point of a move constructor is to take ownership of some resource that's expensive to copy.

In this case, ownership of your data pointer can be transferred from an existing matrix to the newly constructed matrix object. But the idea is to transfer ownership, to the new object, not to share ownership with the new object. In this case that just means setting moved_copy.data to nullptr, that way it won't delete your data when it's destroyed.

matrix::matrix(matrix&& moved_copy)
{
    number_columns = moved_copy.number_columns;
    number_lines = moved_copy.number_lines;
    data = moved_copy.data;
    moved_copy.data = nullptr;
}

Notice that I also removed your if guard: there's no way to construct an object from itself, so that's not really needed for a move constructor (it can be useful for a move assignment operator though).

I also removed the const from moved_copy. Move constructors need to modify the state of the moved-from object to take ownership of its resources, so const cant' be used.

Edit: It is actually possible to construct an object from itself, but it's not something that you really need to guard against.

edited Mar 22 '17 at 11:16

answered Mar 22 '17 at 10:55

Miles Budnek

28,216
2
35
52

yeah you're right, the if condition is not necessary for the move or copy constructor. thank you – M.Azzeddine Mar 22 '17 at 11:13
@Angew, oops, I didn't notice that `const` was there when I copied it from the OP. – Miles Budnek Mar 22 '17 at 11:14
@MilesBudnek `Foo f = f` and `Foo f = std::move(f)` are Undefined Behavior because you use `f` before it is initialized. – bolov Mar 22 '17 at 12:22
@bolov Is it actually undefined if you don't access any attributes of `f`? Or is taking the address of `f` enough? – Miles Budnek Mar 22 '17 at 12:32
@MilesBudnek it's undefined behavior to use the value of an uninitialized variable. Taking it's address it's ok. So `int a = a` is UB. type doesn't matter `T a = a` is undefined no matter what `T` is. `T a = foo(a)` is UB. `T a = foo(a + 3)` is UB. Not that you would want to write something like this, but `void *p = &p;` is OK. `std::size_t a = sizeof(a)` is OK also because `sizeof` arg is unevaluated context. – bolov Mar 22 '17 at 12:39
@bolov, so it sounds like an `if (this != &other)` guarding access to the copied object should prevent UB then, since the value of `other` would never be used. I still don't think it's something worth guarding against, since constructing an object from itself doesn't really make sense anyway. – Miles Budnek Mar 22 '17 at 12:47
@MilesBudnek you are missing the point. Let's strip it down to basics. Let `T` be a class and `b` be an uninitialized variable i.e. `T b;`. Then `T a = b` is Undefined Behavior **no matter what** the copy constructor does. Even if the copy constructor **does absolutely nothing**, even if **it doesn't access `b`** in any way, **it is still UB**. The UB comes from `T a = b` (this level here, not from within the copy constructor). Here you can see that the value of `b` is used in the expression in the right of the `=`. That alone is UB. [to be continued] – bolov Mar 22 '17 at 12:52
... The same logic applies if we replace `b` with `a` (because `a` is uninitialized to the right of `=`) or if we use `std::move(a)`. – bolov Mar 22 '17 at 12:52
An initialized variable can be used **only**: to be initialized (by a valid expression) (i.e. `T a = `), to have it's address taken (i.e. `&a`) or in an unevaluated context (i.e. `sizeof(a)`) . `T a = a` is UB because at the right of `=` `a` is uninitialized and is used in an expression not one of the above categories. – bolov Mar 22 '17 at 12:56

alain · Answer 2 · 2017-03-22T11:20:17.263

0

The problem with this move constructor is that the data member has the same value on both objects after the move, such that when the first object is deleted, the second has a pointer to deleted memory.

Change the move constructor to:

matrix::matrix(matrix&& moved_copy)
{
    if (this != &moved_copy) 
    {
        number_columns = moved_copy.number_columns;
        number_lines = moved_copy.number_lines;
        data = moved_copy.data;
        moved_copy.number_columns = 0;
        moved_copy.number_lines = 0;
        moved_copy.data = nullptr;
    }
}

The check if (this != &moved_copy) could be omitted, because an object is normally not constructed by moving from itself.

edited Mar 22 '17 at 11:20

answered Mar 22 '17 at 10:53

alain

11,939
2
31
51

if `this == &moved_copy` `data` would be lost... Or is it never possible that `this == &moved_copy`? – alain Mar 22 '17 at 10:57
I am pretty sure that this check makes sense only in *copy assignment operator*. It is never possible that `this == &moved_copy` neither in copy constructor nor in move constructor/move assignment operator. – Edgar Rokjān Mar 22 '17 at 11:00
Yes, I guess you're right, I added a note. Maybe it's possible with some placement-new hack, but that would be bad coding and probably not valid anyway. – alain Mar 22 '17 at 11:03
@EdgarRokyan there is some debate about it in move-assignment operator. [see here](http://stackoverflow.com/questions/9322174/move-assignment-operator-and-if-this-rhs) – M.M Mar 22 '17 at 11:04
@M.M I will check it ASAP. Seems that I'm not so right in my comments... – Edgar Rokjān Mar 22 '17 at 11:06

101010 · Answer 3 · 2017-03-22T12:16:21.050

No you should not perform a deep copy in the move constructor. Otherwise you don't gain anything and the notion of a move constructor is broken:

matrix::matrix(matrix&& moved_copy)
: data(moved_copy.data),
  number_rows(moved_copy.number_rows),
  number_columns(moved_copy.number_columns),
  capacity_rows(moved_copy.capacity_rows),
  capacity_columns(moved_copy.capacity_columns) {

  moved_copy.data = nullptr;


}

Furthermore, avoid to define binary operators as member functions because you're breaking the mathematical property of commutativity. That is, although:

matrix M;
...
matrix K = m * 2.0;

will work. The following won't:

matrix M;
...
matrix K = 2.0 * m;

Prefer to define binary operators as free functions.

matrix operator*(matrix const &m, double lambda) {
  matrix out(m.aux_data, m.number_rows, m.number_columns);

  ...

  return out;
}

matrix operator*(double lambda, matrix const &m) {
  return m * lambda;
}

score 0 · Answer 4 · answered Mar 22 '17 at 12:50

I am new to C++11.

So you won't mind me suggesting that you implement your matrix in terms of std::vector, as then all your move concerns are solved for you:

Here's the beginning of one implementation:

#include <vector>
#include <cstddef>
#include <iostream>

class matrix
{

private:

    std::vector<double> storage_;
    std::size_t         row_capacity_;
    std::size_t         rows_;
    std::size_t         cols_;

    std::size_t get_location(std::size_t row, std::size_t col) const
    {
        return row * row_capacity_ + col;
    }

public:
    matrix(std::size_t rows, std::size_t cols, std::size_t row_capacity, std::size_t col_capacity)
        : storage_(row_capacity * col_capacity)
        , row_capacity_(row_capacity)
        , rows_(rows)
        , cols_(cols) {}

    matrix(std::size_t rows, std::size_t cols)
        : matrix(rows, cols, rows, cols) {}

    //
    // note that all move/copy operations are automatically generated.
    // "The rule of none"
    //

    std::size_t get_rows() const { return rows_; }

    std::size_t get_cols() const { return cols_; }


    std::size_t get_capacity_rows() const { return row_capacity_; }

    std::size_t get_capacity_cols() const { return storage_.capacity() / row_capacity_; }

    double& at(std::size_t row, std::size_t col)
    {
        return storage_[get_location(row, col)];
    }

    double const& at(std::size_t row, std::size_t col) const
    {
        return storage_[get_location(row, col)];
    }
};


int main()
{
    auto m = matrix(3, 3);
    m.at(1, 1) = 2;

    std::cout << m.at(1, 1) << std::endl;
    std::cout << m.get_cols() << std::endl;
    std::cout << m.get_rows() << std::endl;
    std::cout << m.get_capacity_cols() << std::endl;
    std::cout << m.get_capacity_rows() << std::endl;
}

you found a bijection between AxA and A [ by : (i,j) --> n*i + j ], i think that in terms of memory this is a lot better than using a vector of vectors (2 pointers instead of 2*n + 2 ones) and a lot safer than using raw pointers and plus i will get to manipulate matrix columns easily just by using modulo operations so thank you very much for your suggestion. — M.Azzeddine, Mar 22 '17 at 15:34

Deep copy in a move constructor

4 Answers4