CODEWITHSUNDEEP
c++memorydynamicdelete-operator
mondaugen
mondaugen

Reputation: 454

Class constructed by passing an external pointer or by internally calling new, can I naively delete this pointer on destruction?

I have class A which has a pointer to class B as one of its fields:

class A {
private:
    B *ptr;
public:
    A(B*);
    A();
    ~A();
}

A::A(B *ptr)
{
    this->ptr = ptr;
}

A::A()
{
    this->ptr = new B();
}

A::~A()
{
    delete this->ptr;
}

I want the destructor to delete B only if it was allocated by new, can I only do this by having a flag that is set storing the origin of the pointer (so that delete is only called if ptr was allocated during construction of A), or is there a more elegant solution, ie, can I just call delete on whatever pointer (I know that if the pointer passed in was allocated using new, that it will also be deleted, but if the pointer is passed in from the scope in which the constructor of A is called, apparently deleting this is undefined)?

Upvotes: 1

Views: 549

Answers (4)

Steve Jessop
Steve Jessop

Reputation: 279255

Aside from what everyone else says (avoid this design): if you absolutely cannot avoid this design, and you need to have this funny class that sometimes owns its B and sometimes doesn't, and which it is depends on which constructor is called, then you could use unique_ptr (or shared_ptr) like so:

void delete_it(B*) { std::default_delete<B>()(t); }
void do_nothing(B *t) {}

class A {
    unique_ptr<B, void(*)(B*)> ptr;
  public:
    A(B *ptr) ptr(ptr, do_nothing) {}
    A() ptr(new B(), delete_it) {}
    // no need for destructor
    // hence also no need to worry about copy constructor or assignment
};

The reason it's better than "raw pointer plus a flag" is that you avoid having to implement deleter and copies (and moves in C++11). It might be worse than "raw pointer plus a flag" for performance, but most of the time you don't need to worry about that. You can come back and do the extra work if it's justified. You don't need to change the source interface of the class to switch, although it would probably break binary compatibility.

Upvotes: 1

David Rodr&#237;guez - dribeas
David Rodr&#237;guez - dribeas

Reputation: 208353

The question cannot be answered just with the code you provided. The real question is why you can either accept a pointer or not, and who owns the memory if a pointer is passed to you.

Depending on what your object should do in your particular domain, then there are different alternatives that could be taken. For example, if you can share the ownership with the caller then you should take a shared_ptr, if you will keep the ownership in the object you should use a unique_ptr making it clear in the interface that your object will grab ownership. If ownership is not to be shared, then consider passing a reference to the object (even if internally you store a pointer), in this case you can separate the ownership with the actual use:

// option 1: shared ownwership
class A {
   std::shared_ptr<B> ptr;
public:
   A( std::shared_ptr<B> const & p ) : ptr(p) {}
   A() : ptr( std::make_shared<B>() ) {}
};
// option 2: grab ownership
class A {
   std::unique_ptr<B> ptr;
public:
   A( std::unique_ptr<B> p ) : ptr( std::move(p) ) {}
   A() : ptr( new B(); }
};
// option 3: no ownership (unless we create the object)
// (separating concerns: ref to access the object, ptr to manage it)
class A {
   std::unique_ptr<B> ptr;   // only ownership
   B * ref;
public:
   A( B & b ) : ptr(), ref(&b) {}
   A() : ptr( new B() ), ref(ptr.get()) {}
   // Internally use 'ref' to access the object the destructor will delete it 
   // if needed
};

Upvotes: 1

Ed Swangren
Ed Swangren

Reputation: 124652

or is there a more elegant solution

No, not really; it needs to be clearly documented as to who owns this pointer. It looks like your class does, so can you just construct ptr yourself?

#include <memory>

class A {
private:
    std::unique_ptr<B> ptr;
public:
    A(some_data args) 
        : ptr(new B(args)) { };
    A();    
}

If your class does not own the pointer that's ok too, just don't deallocate it. Otherwise you will need to set a flag externally (i.e., dealloc_on_destruct) to know what to do, and this will likely get messy.

Upvotes: 3

Mark B
Mark B

Reputation: 96241

The more elegant solution is to not have classes with differing ownership models, in this case owned vs not owned by your class. Don't mix these two ideas in the same class type as it will just confuse your users/clients (by violating the principle of least surprise).

Either create separate classes with the different behaviors you require, or pick an ownership model and force your clients to stick to it (either pass in and transfer ownership in the non-default constructor, or don't allow for self-allocation).

Upvotes: 3

Related Questions