Destructor for class that holds pointer to object

[eagleking95]

Lets say we have a class that holds a pointer member to another object. If I delete that pointer in the destructor I get an error (and I understand why). My question is : is it possible to overcome that without memory leaks ?

 1 #include<iostream>
  2 using namespace std;
  3
  4 class A {
  5     public:
  6     ~A() {  
  7         cout<< "~A()" <<endl;
  8     }
  9 };
 10 
 11 class B {
 12     A *pA;
 13     
 14     public:
 15     
 16     B(A* pA) {
 17         this->pA = pA;
 18     }
 19     
 20     ~B() { 
 21         cout<<"~B()"<<endl;
 22         delete pA;
 23     }
 24 };
 25 
 26 int main() {
 27     
 28     A a;
 29     
 30     {   
 31         B b(&a); //here if I delete a I get an error
 32     }
 33     { B b2(new A()); } //here if I wouldn't delete A I would have memory leaks
 34     return 0;
 35 }            

[cire]

Lets say we have a class that holds a pointer member to another object. If I delete that pointer in the destructor I get an error (and I understand why). My question is : is it possible to overcome that without memory leaks ?

The obvious solution is to use a design where this isn't possible or just forbid users of the class from doing such silly things. B should either own what it points to or not, but it is certainly possible to make it possible for either to work.

A naive implementation might look something like the following:

#include<iostream>
using namespace std;

class A {
public:
    ~A() {  
        cout<< "~A()" <<endl;
    }
};



class B {
    A *pA;
    bool owns_pA ;

public:

    B(A* pA, bool ownIt) 
        : pA(pA), owns_pA(ownIt) {}

    ~B() { 
        cout<<"~B()"<<endl;
        if ( owns_pA )
            delete pA;
    }
};

int main() {

    A a;

    {   
        B b(&a, false); 
    }
    { B b2(new A(), true); } 
    return 0;
} 

[Disch]

This is a design question. It's really about ownership. The best practice I've found is to define a single, clear owner for objects. The owner is in charge of creating the object and doing any necessary cleanup.

So the question is... who owns the A object here? Is it main()? or is it B?


If main() is owning the A object, then B should not be deleting it. It should just take a pointer and refer to the object assuming it will have the necessary lifetime.

If B is the owner, then B must do what is necessary to clean up. But knowing what is necessary means knowing how its created... so it might be better for the A to actually be created in the B class.



I wouldn't recommend keeping it how you have it now (main() allocates, but B owns)... but it could work.

Blah blah blah. Anyway, you have a few options. 3 stand out to me as the "best". One of these 3 are probably what you want, depending on what these classes are used for:

----------------------
1) B does not delete the A object, and it assumes whoever created the A object will destroy it (ie: main() will take care of it -- B is not the owner, main() is).

Pros: The A object can be allocated any which way. Multiple B's can share access to a single A.
Cons: The A object must have a longer lifetime than all B objects which refer to it or else the pointer will go bad.


----------------------
2) B owns the object. By which I mean it creates it and destroys it (or accepts a passed object, but then claims ownership of that object). You can do this without allocating anything with new -- just make a normal non-new'd A object a member of the B class.

Or if you want it new'd... you can accept a pointer, but then you must ALWAYS pass a new'd pointer and can never pass a pointer to a local object like you're doing on line 31.

Pros: No lifetime issues. Since B owns the object, it can keep it alive as long as it needs it.
Cons: One B = One A... multiple B's cannot share the same A. Must be mindful of how objects are created.

----------------------
3) Protected ownership with smart pointers. IE: use shared_ptr or unique_ptr to dictate ownership.

Pros: The A object can be shared by multiple B's. Cleanup is automatic. No lifetime issues.

Cons: A objects must be new'd... no local objects can be used. Must be wary of circular references if A also has a B pointer as a member.




----------------
EDIT:

cire's approach is another possibility. But I find conditional ownership to be a nightmare to maintain, so I recommend against it.

Pick an owner and run with it. Having B own the A sometimes is very error prone and confusing. It does not scale well.

[eagleking95]

Thanks Disch for the clear answer. I guess I have to choose. I'll look into the third solution and see if I can make it work.

[cire]

Disch wrote:

cire's approach is another possibility. But I find conditional ownership to be a nightmare to maintain, so I recommend against it.

For the record, I recommend against it too.

[Disch]

cire's approach actually made me think of something interesting. I wonder if you could try to detect whether or not the pointer was dynamically allocated with something like this:

class B
{
public:
    B( A*& a )
    {  /* new'd - take ownership */ }

    B( A*const& a )
    {  /* stack allocated - do not take ownership */ }

I don't know for certain if this would work... and I know it's not perfect and the wrong version could be called in some circumstances, so I definitely am not saying you should do this. I just thought it was interesting.