views:

232

answers:

6

What is the best approach to encapsulate objects and manage their lifetime? Example: I have a class A, that contains an object of type B and is solely responsible for it.

Solution 1, clone b object to ensure that only A is able to clean it up.

class A
{
    B *b;
public:
    A(B &b)
    {
        this->b = b.clone();
    }

    ~A()
    {
        delete b; // safe
    }
};

Solution 2, directly use the passed object, we risk a potential double free here.

class A
{
    B *b;
public:
    A(B *b)
    {
        this->b = b;
    }

    ~A()
    {
        delete b; // unsafe
    }
};

In my actual case, solution #2 would fit best. However I wonder if this is considered bad code because someone might not know about the behavior of A, even if it's documented. I can think of these scenarios:

B *myB = new B();
A *myA = new A(myB);
delete myB; // myA contains a wild pointer now

or

B *myB = new B();
A *firstA = new A(myB);
A *secondA = new A(myB); // bug! double assignment
delete firstA; // deletes myB, secondA contains a wild pointer now
delete secondA; // deletes myB again, double free

Can I just ignore these issues if I properly document the behavior of A? Is it enough to declare the responsibility and leave it up to the others to read the docs? How is this managed in your codebase?

+2  A: 

If you are writing code that someone else will be using later, these issues must be addressed. In this case I would go for simple reference counting (maybe with smart pointers). Consider the following example:

When an instance of the encapsulating class is assigned an object B, it calls a method to increase object's B reference counter. When the encapsulating class is destroyed, it doesn't delete B, but instead calls a method do decrease reference count. When the counter reaches zero, object B is destroyed (or destroys itself for that matter). This way multiple instances of encapsulating class can work with a single instance of object B.

More on the subject: Reference Counting.

frgtn
So you would change the constructor of A to take something like shared_ptr containing b?
driAn
Reference counting is not needed. As stated A takes ownership.
Martin York
+1  A: 

If you're cloning A, and both A1 and A2 retain references to B, then B's lifetime is not being controlled entirely by A. It's being shared among the various A. Cloning B ensures a one-to-one relationship between As and Bs, which will be easy to ensure lifetime consistency.

If cloning B is not an option, then you need to discard the concept that A is responsible for B's lifetime. Either another object will need to manage the various B, or you'll need to implement a method like reference counting.

For reference, when I think of the term 'Clone', it implies a deep copy, which would clone B as well. I'd expect the two As to be completely detached from each other after a clone.

Nick Gebbie
A: 

I don't clone things unnecessarily or "just to be safe".

Instead I know whose responsibility it is to delete something: either via documentation, or by smart pointers ... for example, if I have a create function which instantiates something and returns a pointer to it and doesn't delete it, so that it's unclear where and by whome that thing is ever supposed to be deleted, then instead of create's returning a naked pointer I might define create's return type as returning the pointer contained within some kind of smart pointer.

ChrisW
+2  A: 

If your object is solely responsible for the passed object then deleting it should be safe. If it is not safe than the assertion that you are solely responsible is false. So which is it? If you're interface is documented that you WILL delete the inbound object, then it is the caller responsibility to make sure you receive an object that must be deleted by you.

jmucchiello
But we can use the auto_ptr to make the contract enforcable and thus stop a lot of bugs. Asking developers to read documentation to use somthing correctly is just aksing for trouble :-)
Martin York
std::auto_ptr is very broken. It is a cure that's worse than the disease. Suggest boost::smart_ptr before even thinking about std::auto_ptr. As for your point, not following my documentation is a bug just like not following Win32 API documentation or man 3 documentation.
jmucchiello
+5  A: 

I never delete anything myself unless I really have to. That leads to errors.

Smart pointers are your friend. std::auto_ptr<> is your friend when one object owns another and is responsible for deleting it when going out of scope. boost::shared_ptr<> (or, now, std::tr1::shared_ptr<>) is your friend when there's potentially more than one object attached to another object, and you want the object deleted when there's no more references to it.

So, either use your solution 1 with auto_ptr, or your solution 2 with shared_ptr.

David Thornley
+4  A: 

You should define your object so that the ownership semantics are, as much as possible, defined by the interface. As David Thornley pointed out, std::auto_ptr is the smart pointer of choice to indicate transfer of ownership. Define your class like so:

class A
{    
    std::auto_ptr<B> b;
public:    
    A(std::auto_ptr<B> b)    
    {
        this->b = b;
    }
    // Don't need to define this for this scenario
    //~A()
    //{ 
    //   delete b; // safe
    //}
};

Since the contract of std::auto_ptr is that assignment = transfer of ownership, your constructor now states implicitly that an A object has ownership of the pointer to B it's passed. In fact, if a client tries to do something with a std::auto_ptr<B> that they used to construct an A after the construction, the operation will fail, as the pointer they hold will be invalid.

Harper Shelby