Isn't it bad to have a copy constructor making shallow copies?

user_9961352 0
Viewed 124

Let's consider that I have defined some classes B and C.

Now, let's consider the following class:

class A{
public:
    int att1;
    int att2;
    std::shared_ptr<B> b_ptr;
    std::shared_ptr<C> c_ptr;
    A(const A& a);
};

Now, the question is about how to implement the copy constructor.

Currently in the code, this is what is being done:

A::A(const A& a){
    att1 = a.att1;
    att2 = a.att2;
    b_ptr = a.b_ptr;
    c_ptr = a.c_ptr;
}

The last two lines will call the copy constructor of std::shared_ptr and increment the number of owners of the underlying pointer by 1.

But, isn't it a bad idea to have a copy constructor do that? It means that anywhere the copy constructor is called, it will actually have a relation with the initial object because the objects of class B and C aren't actually copied, but only the pointers are.

I realize that thanks to the use of shared_ptr, at least this will not be making any memory leaks.

Is it a common practice to do that? Is there any legitimate reason to do it? Or should I just avoid it all together?

pointers smart-pointers c++
3 Answers

Let's imagine class A is class Person and class B is class car.

Is it OK that a copy of class Person points to the same original car? Yes

--- So just copying a pointer is OK.

Is it OK that when the last copy of Person is destroyed the car gets destroyed? Yes

--- So just copying smart pointers is OK.

Is it OK that a copy of class Person will get its own copy of a car? Yes

--- So copying the whole pointed to object is also OK.

All of those semantics can be valid. It is up to you to chose which one is appropriate for your logic.

user_8897420

The last two lines will call the copy constructor of std::shared_ptr and increment the number of owners of the underlying pointer by 1.

Actually, they call the copy assignment operator, not the copy constructor.

But, isn't it a bad idea to have a copy constructor do that?

No, because shared_ptr is meant to be copied like this. Sharing ownership of a pointer across multiple instances is its sole purpose.

It means that anywhere the copy constructor is called, it will actually have a relation with the initial object because the objects of class B and C aren't actually copied, but only the pointers are.

Exactly, yes. That is what shared_ptr is specifically designed for.

Is it a common practice to do that?

In this case, yes. But if raw pointers had been used instead, then no.

user_658630

To answer the question in your subject line, it is neither a good nor a bad thing for a copy constructor to make shallow copies.

There are use cases where deep copies (in the sense of copying creating a clone of all contained objects) are desirable and correct. There are other use cases where shallow copies (not cloning contained objects) are desirable and correct. So it is up to you, as designer of your class, to decide what copying semantics are appropriate.

In your particular example, your class A contains a std::shared_ptr<B> and a std::shared_ptr<C>, and the purpose of std::shared_ptr is to provide shared ownership of a managed object's lifetime i.e. copying (or copy assigning) a std::shared_ptr<X> means the copy shares responsibility for managing lifetime of the same X as the original std::shared_ptr<X>, rather than somehow cloning the managed X and creating a shared_ptr<X> that manages the clone.

The problem in your example is therefore not with the fact that copying an instance of A does not do a deep copy of the B and C that it manages. Assuming you actually seek a deep copy, the problem lies with your choice to use shared_ptr - you have chosen the wrong tool (and then need to write code to do the deep copy).

Incidentally, your copy constructor

A::A(const A& a){
  att1 = a.att1;
  att2 = a.att2;
  b_ptr = a.b_ptr;
  c_ptr = a.c_ptr;
}

does not do a deep copy either. It is also unnecessary since the compiler will generate one for you that has the same net effect as;

A::A(const A& a): att1(a.att1), att2(a.att2), b_ptr(a.b_ptr), c_ptr(a.c_ptr)
{}

This uses copy constructors of std::shared_ptr<B> and std::shared_ptr<C> so differs from your implementation that default-constructs b_ptr and c_ptr first, and then assigns them using std::shared_ptr::operator=().

user_47067850
Related