Managing memory from a class with a pointer to an instance of another class

[bmos31]

I'm having some problems finding memory management errors in this code. The only one I see as a possible error is the deleting the dereferenced pointer, but I'm not even sure if thats correct. Some of the errors should include memory leaks, memory deletion when still in use, two objects pointing to the same object, and memory is allocated for new instances when it shouldnt be. Any help?!

class SomeClass{
public:
  SomeClass(int size) {myMember = new OtherClass(thingSize);};
 ~SomeClass();
 SomeClass(const SomeClass & other);
 SomeClass & operator=(const SomeClass & other);
private:
  OtherClass * myMember;
};
SomeClass::SomeClass(const SomeClass & other){
   myMember = other.myMember;
}
SomeClass::~SomeClass(){
delete * myMember;
}
SomeClass & SomeClass::operator=(const SomeClass & other)
if(this != &other){
myMember = new OtherClass(*other.myMember);
}
return *this
}

[gerard4143]

You have a memory leak in your = operator. You should first delete your member pointer before you create the new one.

Plus you have numerous errors in your example like:

SomeClass(int size) {myMember = new OtherClass(thingSize);};

you have size but use thingSize?

[mike_2000_17]

On a technical side, you have a problem with pointers / references / objects. A pointer is an address to an object of a class. An object is allocated for and created with new and destroyed and deallocated with delete via a pointer to that object (not a dereferenced pointer). A reference is an alias for the object and thus should not and needs not be dereferenced to access the object.

On a more fundamental side, you need to look at issues of object-ownership and deep- vs. shallow-copies. In your example, you are mixing the two. Generally, if an object owns another object (via a pointer to it), it is responsible for deleting it and thus it cannot give that pointer-to-object away to another object without either transferring the ownership or performing a deep-copy (meaning allocating a new object to which the previous one is copied). A very good way to solve those problems is to use smart pointers (unique_ptr, shared_ptr, weak_ptr).

So, the options are (with implementation):
1) An object of class SomeClass owns the object pointed to by myMember:

class SomeClass {
  public:
    SomeClass(int size) : myMember(new OtherClass(size)) { };
    ~SomeClass();
    SomeClass(const SomeClass & other);
    SomeClass & operator=(const SomeClass & other);
  private:
    OtherClass * myMember;
};
/* this is bad because it makes both objects "this" and "other" share ownership (the first to be deleted will corrupt the other).
SomeClass::SomeClass(const SomeClass & other){
  myMember = other.myMember;
  myMember = new OtherClass(*(other.myMember));
}*/

//instead, perform a deep-copy:
SomeClass::SomeClass(const SomeClass & other) : myMember(new OtherClass(*(other.myMember))) { };

SomeClass::~SomeClass(){
  if(myMember) //check that myMember is non-NULL
    delete myMember; //don't dereference, delete operates on the pointer, but deletes whatever it points to.
};

SomeClass& SomeClass::operator=(const SomeClass & other)
  if(this != &other){
    //check that myMember is not already pointing to an object:
    if(myMember)
      delete myMember; //without this, you have a memory leak on your hands.
    myMember = new OtherClass(*(other.myMember)); //this is a deep-copy.
  };
  return *this;
};

For the other cases, I cannot recommend not using smart pointers, so I will show them with smart pointers.
2) The object of class SomeClass only refers to the object pointed to by myMember but claims no ownership over it (external ownership). This is implemented using weak_ptr which can verify whether the pointer has been deleted externally before usage:

class SomeClass {
  public:
    //Pass the pointer to the constructor since SomeClass does not own it, it has to come from the external code (with default value provided).
    SomeClass(std::TR1::weak_ptr<OtherClass> aMember = std::TR1::weak_ptr<OtherClass>()) : myMember(aMember) { };
    ~SomeClass();
    SomeClass(const SomeClass & other);
    SomeClass & operator=(const SomeClass & other);
    //Say you need to access some method of OtherClass:
    void someMethod();
  private:
    std::TR1::weak_ptr<OtherClass> myMember;
};
//shallow-copy is good in this situation.
SomeClass::SomeClass(const SomeClass & other) : myMember(other.myMember) { };

SomeClass::~SomeClass(){
  //delete myMember; //no deletion because there is no ownership claimed by SomeClass.
};

SomeClass & SomeClass::operator=(const SomeClass & other)
  myMember = other.myMember; //no need for anything special here (shallow-copy).
  return *this
};

void someMethod() {
  //here, you first test if the pointer is still valid:
  if(!myMember.expired())
    myMember.lock()->otherMethod(); //lock a shared_ptr to the object and call the otherMethod() on it.
};

3) The objects of class SomeClass share ownership of the object pointed to by myMember. In this case, shared_ptr can be used to count the number of owners (or instances of a shared_ptr to that object) and only delete the object once all owners have let go of the pointer they had.

class SomeClass {
  public:
    SomeClass(int size) : myMember(new OtherClass(size)) { }; //here the shared_ptr requires explicit construction from a normal pointer.
    ~SomeClass();
    SomeClass(const SomeClass & other);
    SomeClass & operator=(const SomeClass & other);
  private:
    std::TR1::shared_ptr<OtherClass> myMember;
};

SomeClass::SomeClass(const SomeClass & other){
   myMember = other.myMember; //this will duplicate the ownership (makes "this" and "other" share ownership of *myMember).
};

SomeClass::~SomeClass(){
  //delete * myMember; //shared_ptr is automatic and gets deleted as it goes out of scope, no explicit "delete" is required or possible.
};

SomeClass & SomeClass::operator=(const SomeClass & other)
  myMember = other.myMember; //again, create a shared ownership relation here.
  return *this
};

You can pick the situation you like. Note as well, that if you like smart pointers (and I hope the last two examples above convinced you), then you should know that option 1 is even more robust by using a unique_ptr (which makes it a compiler error to try and not do a deep copy for myMember).

[bmos31]

thanks you guys, I'm still feeling so new and far behind compared to the majority of my CS class. Thanks to you all for making it so much easier for me to understand!