Best practice on where to delete a resource in an inheritance hierarchy

Consider the following code:

class Base {
protected: 
    int* ptr_;

public:
    virtual ~Base() { /* delete ptr_ ?? */ }    
}

class A : public Base {
public:
    A() { ptr_ = new int; }
    ~A() { /* delete ptr_ ?? */ }
}

I'm having a minor dispute with my colleague.

Which destructor should delete ptr_?

I think it should be in A, because it's where it is allocated.

He thinks it should be in Base, because thats where the pointer is.

Please give possible pitfalls why one method is better than the other (if there is one, and it's not just a matter of taste)

EDIT

Alot of people are questioning the design. In practice, the code is much more complicated, and models an Image processing system involving a few cameras. The role of Base is to manage the resources of the system (A few configurations exist). Class A and other derived types like it decide the configuration of Base and initialize the system to a specific configuration. I guess now you can ask if Inheritance is the right choice over Composition. A is-a 开发者_如何学JAVABase, just a specific kind, and that is why we chose inheritance. In this case, ptr_ is a pointer to a specific driver of a camera, which will be decided by the derived type, that is why it is allocated there.

Each class should manage its own resources. When you have multiple derived classes, each one must remember to free the pointer. This is bad, since it duplicates code.

If some derived class is allowed not to assign the pointer a fresh value, then set it to zero in the base class constructor. If some derived class should be allowed to assign to the pointer the address of an automatic variable, then review your design.

Use smart pointers to make the problem totally go away (when I grep delete * in my entire codebase, I find no match :)

The question is one of design more than one of implementation. I tend to agree with your coworker, if the pointer is in the base, it seems to indicate that the base object is responsible for management of the resource, rather than the derived type.

As a matter of fact the bit that is strange is the fact that the derived constructor is modifying a base member, it probably makes more sense to pass the pointer to the base constructor in which case the semantics would be clearer: the base receives a resource during construction and is responsible to manage it during it's lifetime.

If the base should not manage the resource, then why is the pointer at that level? Why is it not a member of the derived type?

Note that when managing a resource, you should consider the rule of the three¹: If you have to implement one of copy-constructor, assignment-operator or destructor, then you probably want to implement the three of them (additionally consider implementing a no-throw swap which will be handy).

Once you add copy-constructor and assignment-operator to the mix you will probably see that the natural place to manage the resource is there where it is held.

¹ The rule is generic as there are good reasons not to implement all of them always. For example, if you manage your resources inside members that implement RAII (as smart pointers), you might need to provide copy-construction and assignment-operator's as for deep copying semantics, but destruction would not be required. Alternatively, in some contexts you might want to disable *copy-construction* and/or assignments.

In fact, ptr_ should be private! (See Scott Meyers Effective C++, or any primer on object orientation.)

Now, the question doesn’t even pose itself: only the base class can manage its resource. The derived class can only initialise it by passing a request forward to its base (preferably via the constructor):

A() : Base(new int()) { }

I would prefer deleting it in the Base class because,
If I deallocate it in Derived class destructor(which gets called before Base class destructor) then there is a chance that it can get used accidentally in Base class.

Also, The lifetime of the pointer ends within Base class so that should be the place to deallocate it. In fact, I would allocate the pointer within the Base class and not Derived class.

Best is ofcourse to use RAII, and not delte the pointer explicitly, let the Smart pointers take care of deallocating themselves.

You're worrying about code duplication, but I am afraid it clouded your judgement.

I know that DRY get a lot of press (and for good reason) but you misunderstood it. There is a difference between not duplicating code and not duplicating functionality.

DRY is about not duplicating functionality. There might be code patterns that look alike, but are used for different goals, those should not be merged as each goal could shift independently (and un-merging is a nightmare). Their ressemblance is coincidental.

This is the case here. You arbitrarily force a int* ptr_ in the Base (which does not use it) class because surely all derived classes will need it. How do you know that ? At the moment it turns out that all derived classes do need it, but this is a coincidence. They could choose to rely on something else.

Therefore, a sane design is to provide an interface in the base class, and leave it up to each derived class to choose its own implementation:

class Base { public: virtual ~Base() {} };

class D1: public Base {
public:
  D1(Foo const& f): ptr_(new Foo(f)) {}

private:
  std::unique_ptr<Foo> ptr_;
};

class D2: public Base {
public:
  D2(Bar const& f): ptr_(new Bar(f)) {}

private:
  std::unique_ptr<Bar> ptr_;
};

On the other hand, if the Base class was to provide some common functionality that required data members, then of course those could be hoisted up in the Base class implementation... though it could be argued this is premature optimization as, once again, some derived classes might choose to do things differently.

ptr_ shouldn't be in the base class because nothing in the base class uses it. It should be in the derived class.

If you must have it there, I agree with you, it should be deleted in the derived classes desteuctor. The base class can't even know if that pointer was even used, why should it delete it?

The best practice is the object that creates the resource should delete the resource - it is their responsibility.

To me the best is to have the same object in charge of the allocation and the unallocation. So in your code, class A would be in charge of the delete as it performed the new.

But: why did A allocate ptr_ instead of Base? This is the real question here. There's a problem of design to me as ptr_ belongs to Base. Either it should belong to A instead, or Base should be in charge of its memory management.

i would do this, using RAII.

class RaiiResource
{
private:
   int* ptr_;

public:
   RaiiResource()       {       ptr_ = new int;       }
   ~RaiiResource()       {      delete ptr_;       } 
    int* getResource()      {           return ptr_;        }
}

class Base 
{
protected:
    RaiiResource m_raiiresource;

    void anyMethod()
    {
        int* pIntNeeded = m_raiiresource.getResource();/*when you need the resource*/
    }

public:
    virtual ~Base() {}    
}

class A : public Base {
public:
    A() {}
    ~A() { }
    void anyOtherMethod()
    {
        int* pIntNeededAgain = m_raiiresource.getResource(); /*when you need the resource again somewhereelse*/
    }   
}

Another way would be using std::shared_ptr that allows managing the scope of life of objects between different objects, which is the case in your case ( Base and A are sharing the need of the object (int) existence)