Is it good practice to NULL a pointer after deleting it?

Question

I'll start out by saying, use smart pointers and you'll never have to worry about this.

What are the problems with the following code?

Foo * p = new Foo;
// (use p)
delete p;
p = NULL;

This was sparked by an answer and comments to another question. One comment from Neil Butterworth generated a few upvotes:

Setting pointers to NULL following delete is not universal good practice in C++. There are times when it is a good thing to do, and times when it is pointless and can hide errors.

There are plenty of circumstances where it wouldn't help. But in my experience, it can't hurt. Somebody enlighten me.

Answer 1

I've got an even better best practice: Where possible, end the variable's scope!

{
    Foo* pFoo = new Foo;
    // use pFoo
    delete pFoo;
}

Answer 2

If there is more code after the delete, Yes. When the pointer is deleted in a constructor or at the end of method or function, No.

The point of this parable is to remind the programmer, during run-time, that the object has already been deleted.

An even better practice is to use Smart Pointers (shared or scoped) which automagically delete their target objects.

Answer 3

Setting pointers to NULL after you've deleted what it pointed to certainly can't hurt, but it's often a bit of a band-aid over a more fundamental problem: Why are you using a pointer in the first place? I can see two typical reasons:

• You simply wanted something allocated on the heap. In which case wrapping it in a RAII object would have been much safer and cleaner. End the RAII object's scope when you no longer need the object. That's how std::vector works, and it solves the problem of accidentally leaving pointers to deallocated memory around. There are no pointers.
• Or perhaps you wanted some complex shared ownership semantics. The pointer returned from new might not be the same as the one that delete is called on. Multiple objects may have used the object simultaneously in the meantime. In that case, a shared pointer or something similar would have been preferable.

My rule of thumb is that if you leave pointers around in user code, you're Doing It Wrong. The pointer shouldn't be there to point to garbage in the first place. Why isn't there an object taking responsibility for ensuring its validity? Why doesn't its scope end when the pointed-to object does?

Answer 4

Firstly, there are a lot of existing questions on this and closely related topics, for example http://stackoverflow.com/questions/704466/why-doesnt-delete-set-the-pointer-to-null.

In your code, the issue what goes on in (use p). For example, if somewhere you have code like this:

Foo * p2 = p;

then setting p to NULL accomplishes very little, as you still have the pointer p2 to worry about.

This is not to say that setting a pointer to NULL is always pointless. For example, if p were a member variable pointing to a resource who's lifetime was not exactly the same as the class containing p, then setting p to NULL could be a useful way of indicating the presence or absence of the resource.

Answer 5

Let me expand what you've already put into your question.

Here's what you've put into your question, in bullet-point form:

---

Setting pointers to NULL following delete is not universal good practice in C++. There are times when:

• it is a good thing to do
• and times when it is pointless and can hide errors.

---

However, there is no times when this is bad! You will not introduce more bugs by explicitly nulling it, you will not leak memory, you will not cause undefined behaviour to happen.

So, if in doubt, just null it.

Having said that, if you feel that you have to explicitly null some pointer, then to me this sounds like you haven't split up a method enough, and should look at the refactoring approach called "Extract method" to split up the method into separate parts.

Answer 6

There is always Dangling Pointers to worry about.

Answer 7

Setting a pointer to 0 (which is "null" in standard C++, the NULL define from C is somewhat different) avoids crashes on double deletes.

Consider the following:

Foo* foo = 0; // Sets the pointer to 0 (C++ NULL)
delete foo; // Won't do anything

Whereas:

Foo* foo = new Foo();
delete foo; // Deletes the object
delete foo; // Segfault

In other words, if you don't set deleted pointers to 0, you will get into trouble if you're doing double deletes. An argument against setting pointers to 0 after delete would be that setting pointers to 0 after deleting them just leaves double delete bugs undetected. It's up to you, really: Do you want your application to crash on a double free or not?

On a sidenote regarding managing object allocation, I suggest you take a look at std::auto_ptr or another smart pointer implementation, depending on your needs.

Answer 8

If you're going to reallocate the pointer before using it again (dereferencing it, passing it to a function, etc.), making the pointer NULL is just an extra operation. However, if you aren't sure whether it will be reallocated or not before it is used again, setting it to NULL is a good idea.

As many have said, it is of course much easier to just use smart pointers.

Edit: As Thomas Matthews said in this earlier answer, if a pointer is deleted in a destructor, there isn't any need to assign NULL to it since it won't be used again because the object is being destroyed already.

Answer 9

I'll change your question slightly:

Would you use an uninitialized pointer? You know, one that you didn't set to NULL or allocate the memory it points to?

There are two scenarios where setting the pointer to NULL can be skipped:

• the pointer variable goes out of scope immediately
• you have overloaded the semantic of the pointer and are using its value not only as a memory pointer, but also as a key or raw value. this approach however suffers from other problems.

Meanwhile, arguing that setting the pointer to NULL might hide errors to me sounds like arguing that you shouldn't fix a bug because the fix might hide another bug. The only bugs that might show if the pointer is not set to NULL would be the ones that try to use the pointer. But setting it to NULL would actually cause exactly the same bug as would show if you use it with freed memory, wouldn't it?

Answer 10

Yes.

The only "harm" it can do is to introduce inefficiency (an unnecessary store operation) into your program - but this overhead will be insignificant in relation to the cost of allocating and freeing the block of memory in most cases.

If you don't do it, you will have some nasty pointer derefernce bugs one day.

I always use a macro for delete:

#define SAFEDELETE(ptr) { delete(ptr); ptr = NULL; }

(and similar for an array, free(), releasing handles)

You can also write "self delete" methods that take a reference to the calling code's pointer, so they force the calling code's pointer to NULL. For example, to delete a subtree of many objects:

static void TreeItem::DeleteSubtree(TreeItem *&rootObject)
{
    if (rootObject == NULL)
        return;

    rootObject->UnlinkFromParent();

    for (int i = 0; i < numChildren)
       DeleteSubtree(rootObject->child[i]);

    delete rootObject;
    rootObject = NULL;
}

edit

Yes, these techniques do violate some rules about use of macros (and yes, these days you could probably achieve the same result with templates) - but by using over many years I never ever accessed dead memory - one of the nastiest and most difficult and most time consuming to debug problems you can face. In practice over many years they have effectively eliminated a whjole class of bugs from every team I have introduced them on.

There are also many ways you could implement the above - I am just trying to illustrate the idea of forcing people to NULL a pointer if they delete an object, rather than providing a means for them to release the memory that does not NULL the caller's pointer.

Of course, the above example is just a step towards an auto-pointer. Which I didn't suggest because the OP was specifically asking about the case of not using an auto pointer.

Answer 11

As others have said, delete ptr; ptr = 0; is not going to cause demons to fly out of your nose. However, it does encourage the usage of ptr as a flag of sorts. The code becomes littered with delete and setting the pointer to NULL. The next step is to scatter if (arg == NULL) return; through your code to protect against the accidental usage of a NULL pointer. The problem occurs once the checks against NULL become your primary means of checking for the state of an object or program.

I'm sure that there is a code smell about using a pointer as a flag somewhere but I haven't found one.

Answer 12

If you have no other constraint that forces you to either set or not set the pointer to NULL after you delete it (one such constraint was mentioned by Neil Butterworth), then my personal preference is to leave it be.

For me, the question isn't "is this a good idea?" but "what behavior would I prevent or allow to succeed by doing this?" For example, if this allows other code to see that the pointer is no longer available, why is other code even attempting to look at freed pointers after they are freed? Usually, it's a bug.

It also does more work than necessary as well as hindering post-mortem debugging. The less you touch memory after you don't need it, the easier it is to figure out why something crashed. Many times I have relied on the fact that memory is in a similar state to when a particular bug occurred to diagnose and fix said bug.

Answer 13

My argument against it:

It hides real errors.
Error that could be exposed with profile tools will now not get caught. Now this is fine for the current version of the code. But later versions may re-expose the error and then generate a real problem.

A quick example:

Double delete:

int*  p    = createAPointer();

doGoodStuffWithP(p);

delete  p;
p = NULL;

.....

doBadStuffWithP(p);  // suppose there is a delete in here?

There is a bug in doBadStuffWithP() causing a second call to delete.
This is now not going to be exposed by testing becuase deleting NULL is not a problem.

This probably will not matter to much in version 1.
But modifications are done a year latter and p is now re-used adter the delete and the use of p continues after doBadStuffWithP() now you have a potential to use p after it has been deleted. Because we know that testing on version 2 is so much better than testing on version 1.

So I think it is a bas idea.
Admitadely I prefer to make sure scope kills p so I don't need to make it NULL.

But the hding of error scares me and I would prefer to find them in the first place.

Answer 14

I always set a pointer to NULL after deleting it.

1. It can help catch many references to free'd memory (assuming your platform faults on a deref of NULL).

2. It won't catch all references to free'd memory, for example, if you have a copy of the pointer lying around. But some is better than none.

3. It will mask a double-delete, but I find those are far less common than accesses to already free'd memory.

4. In many cases the compiler is going to optimize it away. So the argument that it's unnecessary doesn't persuade me.

5. If you're already using RAII, the there aren't many deletes in your code to begin with, so the argument that the extra assignment causes clutter doesn't persuade me.

6. It's often convenient, when debugging, to see the NULL value rather than a stale pointer.

7. If this still bothers you, use a smart pointer or a reference instead.

I also set other types of resource handles to the no-resource value when the resource is free'd (which is typically only in the destructor of an RAII wrapper written to encapsulate the resource).

I worked on a large (9 million statements) commercial product (primarily in C). At one point, we used macro magic to NULL out the pointer when memory was freed. This immediately exposed lots of lurking bugs that were promptly fixed. As far as I can remember, we never had a double-free bug.

Answer 15

Explicitly nulling after delete strongly suggests to a reader that the pointer represents something which is conceptually optional. If I saw that being done, I'd start worrying that everywhere in the source the pointer gets used that it should be first tested against NULL.

If that's what you actually mean, it's better to make that explicit in the source using something like boost::optional

optional<Foo*> p (new Foo);
// (use p.get(), but must test p for truth first!...)
delete p.get();
p = optional<Foo*>();

But if you really wanted people to know the pointer has "gone bad", I'll pitch in 100% agreement with those who say the best thing to do is make it go out of scope. Then you're using the compiler to prevent the possibility of bad dereferences at runtime.

That's the baby in all the C++ bathwater, shouldn't throw it out. :)

Answer 16

I would almost set my deleted (or freed in my case) pointers to a garbage value:

char *ptr = malloc(...);
...
...
free(ptr);
ptr = 0xDEADBEEF;

This (probably) takes your pointer away from the heap, so it won't corrupt everything if you double-delete and don't crash. It will also fail (as it should) if someone tries to use it -- AND! It's easy to recognize when debugging, so you know when you see it: "Oh, I'm trying to dereference a pointer that I previously deleted".

That being said, this could be a really bad idea for some reason I haven't thought of, but it seems okay in my mind.

Answer 17

I can imagine setting a pointer to NULL after deleting it being useful in rare cases where there is a legitimate scenario of reusing it in a single function (or object). Otherwise it makes no sense - a pointer needs to point to something meaningful as long as it exists - period.

Answer 18

If the code does not belong to the most performance-critical part of your application, keep it simple and use a shared_ptr:

shared_ptr<Foo> p(new Foo);
//No more need to call delete

It performs reference counting and is thread-safe. You can find it in the tr1 (std::tr1 namespace, #include < memory >) or if your compiler does not provide it, get it from boost.