Is it necessary to use 'delete' on pointers before assigning different value?

Question

This might be a silly question but I am not sure what is the correct way of doing this (searching did not yield definitive answer for me).

Consider following snippet:

class D;

class C
{
    C() {m_Ptr = new D;};
    ~C() {delete m_Ptr;};

    public:
        void setPtr(const D &obj) {*m_Ptr = obj};

    private:
        D *m_Ptr;
};

Is it okay to simply assign new value to *m_Ptr even though it already points to a different object of class D? I tend to use:

void C::setPtr(const D &obj)
{
    delete m_Ptr;
    *m_Ptr = obj;
}

but I am not sure if that is necessary or good/bad practice. My reasoning is that every 'new' should have its 'delete' (or other means of deallocating memory such as parent-child mechanism in Qt).

Thanks for clarification!

Every new should indeed have its delete, but there is no `new` in `*m_Ptr = obj;` — Tim, Mar 26 '14 at 06:59
Yes, your pattern is somewhat unusual in that you allocate dynamically _once_ and then just copy over the allocated _object_. So your class doesn't leak and you _must not_ call delete on that pointer, but you 'll confuse most ppl looking at it: Why not just declare a member of type D? — Peter - Reinstate Monica, Mar 26 '14 at 07:00

score 4 · Answer 1 · answered Mar 26 '14 at 06:58

You might need to do delete if you reassign the pointer. In your case you don't do that, you just change the data pointed to by the pointer.

In fact, doing e.g.

delete m_Ptr;
*m_Ptr = obj;

would lead to undefined behavior as you dereference the pointer after you free the memory it pointed to, and you then write to memory you no longer "own".

score 1 · Answer 2 · answered Mar 26 '14 at 06:59

Every time you new something you have to delete it. This rule is only really broken with something like the std smart pointers which delete the memory for you.

HOWEVER in your example you're simply filling the memory at m_Ptr with the memory that's at obj. So, deleting here would cause problems.

EDIT: As Peter said in the comments, you're far better off storing a D object on the class. Avoid unnecessary pointers like this as much as you can.

score 1 · Accepted Answer · edited May 23 '17 at 11:57

There is a deeper problem than what you're facing now.

void setPtr(const D &obj) {*m_Ptr = obj};

How do you know if obj is an object created by new? How can you make sure the caller intends that C call delete on its address? What you are doing is bad class design, and there is a much better solution to it that both addresses your current issue and what I'm fussing about.

Make sure first that you have read What is The Rule of Three?

So, how can you make your class design better? Use the Rule of Three! Provide copy (and if available move) constructors and assignment operators. After you have done this, remove setPtr. setPtr is a bad thing. Provide a constructor instead that copies an existing D object. In this way, you insulate the details on how you create what is pointed by m_Ptr.

C(D& d) {m_Ptr = new D(d); };

You should also provide an appropriate copy constructor for D. To use it, instead of

C x;
D* y = new D;
x.setPtr(*D);

you do

C x = D();
// or
C x;
x = C(D());

Do you want C to hold a D depending on some parameters? Maybe they are parameters to D's constructor(s)? You could provide a "named constructor":

class C {
    static C createBlah(int x, float y) {
        C c = D(x, y);
        return c;
    }
};

// Using it...
C c = C::createBlah(42, 3.14f);

So now, the users of your class C doesn't have to worry about anything concerning D. Why put the burden on them? Why have them use new when it'll put them in grave danger? Why make the world more miserable?!

There, that's better.

UPDATE: Much better though, make the D object directly as a member, instead of a pointer to it. @Peter's comment pretty much sums up the point.

Is it necessary to use 'delete' on pointers before assigning different value?

3 Answers3