Unusual destructor behaviour when copying over stack variables

Question

I wrote a test to check whether destructors were called before an overwriting assignment on a stack variable, and I can't find any rational explanation for the results...

This is my test (in Visual C++ 2008 Release mode):

#include <iostream>
class C {
public:
 char* ptr;
 C(char p) { ptr = new char[100]; ptr[0] = p;}
 ~C() { std::cout << ptr[0] << ' '; delete [] ptr; }
};

int _tmain(int argc, _TCHAR* argv[])
{
 {
  C s('a');
  s = C('b');
  s = C('c');
  s = C('d');
 }
 std::cin.get();
 return 0;
}

I was expecting to get either "a b c d " if my hypothesis was true, or just "d " if false. Instead I get "b c d x ". The "x " changes depending on how much memory is allocated to ptr indicating that it's reading random heap values.

I believe what is happening (correct me if I'm wrong) is that each constructor call creates a new stack value(lets call them s1, s2, s3, s4), and then the assignments leave s1.ptr overwritten by s4.ptr. s4 is then destroyed immediately after the copy but s1(with a dangling ptr) is destroyed upon leaving the scope, causing a double delete of s4.ptr and no delete of the original s1.ptr.

Is there any way around this unhelpful behavior that doesn't involve using shared_ptrs?

edit: replaced 'delete' with 'delete []'

Answer 1

You could create a copy constructor and assignment operator, as you should do for any type which owns raw pointers.

Answer 2

s only gets destroyed when going out of scope - and, as you mention, gets overwritten over the course of the program, so the initial allocation is leaked and the last one is double-deleted.

The solution is to overload the assignment operator (and, as Pete suggests, provide a copy constructor as they go hand in hand), in which you'll clean the array you have and the copy the one you're given.

Answer 3

Rule of Three

Your application behavior is undefined, since as stated multiple objects will share access to a common pointer and will attempt to read it...

The rule of three states that each time you define one of:

• copy constructor
• assignment operator
• destructor

Then you should define the other, since your object has a specific behavior that the default generated methods don't know about.

EDIT special exception:
sometimes you only define the destructor because you want it virtual, or because it logs something, not because there is some special handling of your attributes ;)

Answer 4

Since you print in the destructor, the a instance will be deleted at the end of the scope (the x you are seeing).

The other instances will be deleted as soon as the assignation is made. that explain the bcdx.

next use the

delete [] ptr;

instead of delete

Answer 5

The problem is that you need copy constructors and assignment operators. Due to the line where you assign one class to the other, a shallow copy is made. This will result in both classes to have the same ptr pointer. If then one of them gets deleted, the other one points top already freed memory

Answer 6

You haven't defined an assignment or copy operator. So whats happening is something like:

C s('a');

The 's' instance gets created and initialized with 'a'.

s = C('b');

This creates a temporary object, initializes it with 'b', and then the default assignment operator kicks in which does a bitwise copy of all variables, overwriting the ptr of s. The temporary object is destroyed. Emitting 'b' and deleting 'ptr' (rendering ptr in s invalid).

s = C('c');
s = C('d');

Same again. Temporary object is created, initialized with 'c', and the 'ptr' in s is overwritten with the ptr allocated in the temporary object. The temporary object is destroyed, emitting 'c' and leaving the ptr in s invalid. Repeat for d.

  return 0;
}

Finally s leaves scope, its destructor tries to emit the first character of ptr, but thats garbage because ptr was allocated and deleted by the last ('d') temporary object. The attempt to delete ptr should fail as that memory is already deleted.

To solve this? Define explicit copy constructors and assignment operators.

class C {
  // other stuff
  C(const C&rhs); // copy constructor
  C& operator=(const c& rhs){ // assignment operator
    a[0] = rhs.a[0];
    return *this;
  }
};

Answer 7

Add the othere compiler defined methods:

class C
{
    public:
      char* ptr;
      C(char p)                { ptr = new char[100]; ptr[0] = p;}
     ~C()                      { std::cout << ptr[0] << ' '; delete [] ptr; }
      C(C const& c)            { ptr = new char[100]; ptr[0] = c.ptr[0];}
      C& operator=(C const& c) { ptr[0] = c.ptr[0]; return *this;}
};

int _tmain(int argc, _TCHAR* argv[])
{
  {
      C s('a');
      s = C('b');
      s = C('c');
      s = C('d');
  }
  std::cin.get();
  return 0;
}

It should now print out:

b c d d

Each temporary gets destroyed at the end of the expression. Then s gets destroyed last (after having 'd' copied into ptr[0]). If you stick a print statement in each method it beomes easier to see what is happening:

>>           C s('a');
Construct 'a'

>>           s = C('b');
Construct 'b'  
Assign 'b' onto 'a'  
Destroy 'b'         (Temporary destroyed at ';')  

>>          s = C('c');
Construct 'c'  
Assign 'c' onto 'b' (was 'a' but has already been assigned over)  
Destroy 'c'         (Temporary destroyed at ';')

>>          s = C('d');  
Construct 'd'  
Assign 'd' onto 'c'  
Destroy 'd'         (Temporary destroyed at ';')  

>> End of scope.
Destroy 'd'         (Object s destroyed at '}')

Since there are 4 methods defined by the compiler the "rule of four" applies.
If your class contains a RAW pointer that is owned by the class (owned means your object determines the life span). Then you must override all 4 compiler generated methods.

Since you create and destroy the member 'ptr' this is an owned ptr. Thus all four methods must be defined.