8

So I stumbled upon this bit of code and while I've been coding in C/C++ for about 5 years now, I cannot fathom why anybody would want to do this. I understand why you'd want to set the pointer to NULL after deallocating memory, but I certainly don't understand why someone would want to do the opposite (looks like a memory leak to me).

Second, I'm pretty sure it's not necessary to check if the pointer is NULL before setting it to NULL and deleting it, as discussed here.

if( m_pio )
{
    m_pio = NULL;
    delete m_pio;
}
Community
  • 1
  • 1
audiFanatic
  • 2,014
  • 5
  • 27
  • 50
  • 12
    That's a memory leak implementation... probably accidentally moved one line up or down. – dtech Feb 12 '16 at 20:20
  • 4
    My _guess_ is this is the result of a bad 3-way merge, a quick and dirty edit, or some kind of refactoring that wasn't reviewed. I've made mistakes similar to this myself resulting in obviously nonsensical (but still functioning) code whose origin was only apparent after digging through the source history. – Chris Schmich Feb 12 '16 at 20:21
  • 4
    .. Also they did not even need the `if` statement. Deleting null is fine – Ed Heal Feb 12 '16 at 20:22
  • I guess this is to prevent double deletes – Tejas Pawar Feb 12 '16 at 20:25
  • @EdHeal Yup, my point exactly in the second part. This codebase is horrendous haha, and this is the "good stuff", believe it or not. – audiFanatic Feb 12 '16 at 20:25
  • 1
    @audiFanatic - there is plenty of "professional grade" software with horrendous code base. This is nothing compared to the atrocities I've seen. – dtech Feb 12 '16 at 20:27
  • Yes, in proper code, the if-statement is not necessary, but it can be helpful in debugging, as it lets you put a full-speed breakpoint on a particular case without using a condition. – Adrian McCarthy Feb 12 '16 at 20:28
  • 2
    @TejasPawar: If `m_pio = NULL` was **after** the `delete`, then it would prevent double `delete`s successfully. As it is, though, it's just a memory leak. – R_Kapp Feb 12 '16 at 20:34
  • Why don't you ask the person who wrote it? oh wait, right, he/she is fired – ForeverStudent Feb 12 '16 at 20:37
  • @ForeverStudent Yep, pretty much, there were too many cooks in the kitchen for this codebase and it was thrown in my lap as the sole developer.... – audiFanatic Feb 12 '16 at 20:49

3 Answers3

7

Double deleting an object can result in the destructor being called twice, so some people favor setting it to NULL after deletion. This prevents the destructor from being called on memory which could have been reallocated from the pointer's previous memory address.

As shown here though, setting to NULL before deleting is a memory leak. Fixed code without memory leak:

if( m_pio ) {
    delete m_pio;
    m_pio = NULL;
}

Note that calling delete on NULL (or nullptr in C++11), is legal so you code could just be written as this instead:

delete m_pio;
m_pio = NULL
Nacho
  • 1,036
  • 1
  • 12
  • 28
pyj
  • 1,311
  • 9
  • 16
  • I prefer to set a deleted pointer to NULL as a debugging aid so I know when a pointer is valid or not. Preventing double deletes or double destruction is a side benefit. – Mark Ransom Feb 12 '16 at 20:51
  • I learned C++ "in the old days", but much of what I'm (re)learning now is to go with `shared_ptr` or `unique_ptr` if you can avoid the overhead. With those and move semantics, some cases where I used to use raw pointers are now avoided. – pyj Feb 12 '16 at 22:13
  • I do the same thing, and `vector` is useful too when all you needed the pointer for was a variable size array. But there are still times when a raw pointer is useful. – Mark Ransom Feb 12 '16 at 22:17
  • @MarkRansom, I agree, being close to metal is why we write in C++. But it's also designed to protect your higher level design too, which is why I said "some cases" ;) I typically run into raw pointer stuff in graphics code. – pyj Feb 12 '16 at 22:19
1

The author has wanted to write:

delete m_pio;
m_pio = NULL;

but messed the lines and there is bad bug. Assigning NULL after delete is good prectice to protect the variable from another delete.

i486
  • 6,083
  • 3
  • 21
  • 37
0

Obviously we can't discern what the original author had intended.

On a side note, you'd be surprised at the amount of people I've worked with (even industry veterans) who have it in their head that deleteing or freeing NULL is somehow a bad thing that needs to be guarded against, despite that there are no consequences for it. It just ends up creating a huge tidal wave of unnecessary checks, especially when the developer in question is especially fond of wasting their time manually managing memory.

Community
  • 1
  • 1
Colin Basnett
  • 3,735
  • 1
  • 27
  • 46