views:

953

answers:

3

Say I have a struct "s" with an int pointer member variable "i". I allocate memory on the heap for i in the default constructor of s. Later in some other part of the code I pass an instance of s by value to some function. Am I doing a shallow copy here? Assume I didn't implement any copy constructors or assignment operators or anything for s... just the default constructor.

+5  A: 

Yes, that's a shallow copy. You now have two copies of s (one in the caller, one on the stack as a parameter), each which contain a pointer to that same block of memory.

Don Neufeld
+1  A: 

You will have two copies of the s struct, each of which will have their own i pointer, but both i pointers will have the same value pointing to the same address in memory - so yes, it will be a shallow copy.

Claudiu
+8  A: 

To follow up on what @[don.neufeld.myopenid.com] said, it is not only a shallow copy, but it is either (take your pick) a memory leak or a dangling pointer.

// memory leak (note that the pointer is never deleted)
class A
{
  B *_b;
  public:
  A()
  : _b(new B)
  {
  }
};

// dangling ptr (who deletes the instance?)
class A
{
  B *_b;
  public:
  A()
  ... (same as above)

  ~A()
  {
    delete _b;
  }
};

To resolve this, there are several methods.

Always implement a copy constructor and operator= in classes that use raw memory pointers.

class A
{
  B *_b;
  public:
  A()
  ... (same as above)

  ~A()
  ...

  A(const A &rhs)
  : _b(new B(rhs._b))
  {
  }

  A &operator=(const A &rhs)
  {
    B *b=new B(rhs._b);
    delete _b;
    _b=b;
    return *this;
};

Needless to say, this is a major pain and there are quite a few subtleties to get right. I'm not even totally sure I did it right here and I've done it a few times. Don't forget you have to copy all of the members - if you add some new ones later on, don't forget to add them in too!

Make the copy constructor and operator= private in your class. This is the "lock the door" solution. It is simple and effective, but sometimes over-protective.

class A : public boost::noncopyable
{
  ...
};

Never use raw pointers. This is simple and effective. There are lots of options here:

  • Use string classes instead of raw char pointers
  • Use std::auto_ptr, boost::shared_ptr, boost::scoped_ptr etc

Example:

// uses shared_ptr - note that you don't need a copy constructor or op= - 
// shared_ptr uses reference counting so the _b instance is shared and only
// deleted when the last reference is gone - admire the simplicity!
// it is almost exactly the same as the "memory leak" version, but there is no leak
class A
{
  boost::shared_ptr<B> _b;
  public:
  A()
  : _b(new B)
  {
  }
};
1800 INFORMATION
Your assignment operator is not exception safe. See the recent question about copy and exception safety: http://stackoverflow.com/questions/214891/checklist-for-writing-copy-constuctor-and-assignment-operator-in-c#214966
Luc Hermitte
Doh, ok I fixed that. You see what I mean about it being a pain and hard to get right then huh
1800 INFORMATION
The best way to make the copy constructor and operator= private is to inherit from boost::noncopyable. You should do that for every single class unless you know for sure it will be copyable.
CesarB
With the copy-and-swap idiom, it's really easy to do it right (even if it is a little inefficient)
Luc Hermitte