Is this C++ reassignment valid?

Question

Sorry for the basic question, but I'm having trouble finding the right thing to google.

#include <iostream>
#include <string>
using namespace std;

class C {
public: 
C(int n) {
    x = new int(n);
}

~C( ) {
    delete x;
}

int getX() {return *x;}

private: 
    int* x; 
}; 

void main( )  {
    C obj1 = C(3);
    obj1 = C(4);
    cout << obj1.getX() << endl;
}

It looks like it does the assignment correctly, then calls the destructor on obj1 leaving x with a garbage value rather than a value of 4. If this is valid, why does it do this?

Answer 1

If there is a class C that has a constructor that takes an int, is this code valid?

C obj1(3);
obj1=C(4);

Assuming C has an operator=(C) (which it will by default), the code is valid. What will happen is that in the first line obj1 is constructed with 3 as a the parameter to the constructor. Then on the second line, a temporary C object is constructed with 4 as a parameter and then operator= is invoked on obj1 with that temporary object as a parameter. After that the temporary object will be destructed.

If obj1 is in an invalid state after the assignment (but not before), there likely is a problem with C's operator=.

Update: If x really needs to be a pointer you have three options:

1. Let the user instead of the destructor decide when the value of x should be deleted by defining a destruction method that the user needs to call explicitly. This will cause memory leaks if the user forgets to do so.
2. Define operator= so that it will create a copy of the integer instead of a copy of the value. If in your real code you use a pointer to something that's much bigger than an int, this might be too expensive.
3. Use reference counting to keep track how many instances of C hold a pointer to the same object and delete the object when its count reaches 0.

Answer 2

When are you testing the value of obj1? Is it after you leave the scope?

In your example, obj1 is a stack object. That means as soon as you leave the function in which it defined, it gets cleaned up (the destructor is called). Try allocating the object on the heap:

C *obj1 = new C(3);
delete obj1;
obj1 = new C(4);

Answer 3

NEW: What's happening is that your destructor has deallocated the memory allocated by the constructor of C(4). So the pointer you have copied over from C(4) is a dangling pointer i.e. it still points to the memory location of the deallocated memory

class C {
public: 
C(int n) {
    x = new int(n);
}

~C( ) {
    //delete x; //Don't deallocate
}

void DeallocateX()
{
 delete x;
}

int getX() {return *x;}

private: 
    int* x; 
}; 

int main(int argc, char* argv[])
{
    // Init with C(3)
    C obj1 = C(3);
    // Deallocate C(3)
    obj1.DeallocateX();
    // Allocate memory and store 4 with C(4) and pass the pointer over to obj1
    obj1 = C(4);
    // Use the value
    cout << obj1.getX() << endl;
    // Cleanup
 obj1.DeallocateX();


 return 0;
}

Answer 4

If C contains a pointer to something, you pretty much always need to implement operator=. In your case it would have this signature

class C
{    
    public:
    void operator=(const C& rhs)
    {
        // For each member in rhs, copy it to ourselves
    } 
    // Your other member variables and methods go here...
};

Answer 5

I do not know enough deep, subtle C++ to explain the problem you are encountering. I do know, however, that it's a lot easier to make sure a class behaves the way you expect if you follow the Rule of Three, which the code you posted violates. Basically, it states that if you define any of the following you should define all three:

• Destructor
• Copy constructor
• Assignment operator

Note as well that the assignment operator implementation needs to correctly handle the case where an object is assigned to itself (so-called "self assignment"). The following should work correctly (untested):

#include <iostream>
#include <string>
using namespace std;

class C {
public: 
C(int n) {
    x = new int(n);
}

C(const C &other): C(other.getX()) { }

~C( ) {
    delete x;
}

void operator=(const C &other) {
    // Just assign over x.  You could reallocate if you first test
    // that x != other.x (the pointers, not contents).  The test is
    // needed to make sure the code is self-assignment-safe.
    *x = *(other.x);
}

int getX() {return *x;}

private: 
    int* x; 
}; 

void main( )  {
    C obj1 = C(3);
    obj1 = C(4);
    cout << obj1.getX() << endl;
}

Answer 6

Be explicit about ownership of pointers! auto_ptr is great for this. Also, when creating a local don't do C obj1 = C(3) that creates two instances of C and initializes the first with the copy constructor of the second.

Heed The Guru.

class C {
public:
   C(int n) : x(new int(n)) { }
   int getX(){ return *x; }
   C(const C& other) : x(new int(*other.x)){}
   C& operator=(const C& other) { *x = *other.x; return *this; }
private:
   std::auto_ptr<int> x;
};

int main() {
   C obj1(3);
   obj1 = C(4);
   std::cout << obj1.getX() << std::endl;
}

Answer 7

Basically you are trying to re-implement a smart pointer.
This is not trivial to get correct for all situations.

Please look at the available smart pointers in the standard first.

A basic implementation (Which will fail under certain situations (copy one of the standard ones to get a better one)). But this should cover the basics:

class X
{
    int*     data;

    public:
      // Destructor obvious
      ~X()
      {
          delete data;
      }
      // Easy constructor.
      X(int x)
          :data(new int(x))
      {}
      // Copy constructor.
      // Relatively obvious just do the same as the normal construcor.
      // Get the value from the rhs (copy). Note A class is a friend of
      // itself and thus you can access the private members of copy without
      // having to use any accessor functions like getX()
      X(X const& copy)
          :data(new int(copy.x))
      {}
      // Assignment operator
      // This is an example of the copy and swap idiom. This is probably overkill
      // for this trivial example but provided here to show how it is used.
      X& operator=(X const& copy)
      {
          X tmp(copy);
          this->swap(tmp);
          return this;
      }
      // Write a swap() operator.
      // Mark it is as no-throw.
      void swap(X& rhs) throws()
      {
          std::swap(data,rhs.data);
      }

  };