Code::Blocks Forums

Hi got a tiny question about app.cpp.

Why this?


void CodeBlocksApp::HideSplashScreen()
{
    if (m_pSplash)
        delete m_pSplash;
    m_pSplash = 0;
}


I belive it should be this....


void CodeBlocksApp::HideSplashScreen()
{
    if (m_pSplash)
    {
        delete m_pSplash;
        m_pSplash = 0;
    }
}


It seems to me that m_pSplash is getting set to 0 unnessecarily.....

Patch below fixes the problem (I think).....  :lol:

[attachment deleted by admin]

If you don't zero the pointer after deleting, you may delete it twice (as HideSplashScreen can be called more often than once).

Deleting the same object twice is a MCA.


EDIT:
Hmm... after reading your post more carefully, you do zero the pointer, sorry ;)
Yes, you are maybe right, it might be zeroed more often than needed, but it really does not matter here.

Actually, it's perfectly OK to delete a null pointer. So maybe it should be

void CodeBlocksApp::HideSplashScreen()
{
    delete m_pSplash;
    if (m_pSplash)
        m_pSplash = 0;
}

?

However, checking the if probably takes more time than the assignment would take, so

void CodeBlocksApp::HideSplashScreen()
{
    delete m_pSplash;
    m_pSplash = 0;
}

is probably even better.

Quote from: thomas on January 24, 2006, 09:29:41 PM
If you don't zero the pointer after deleting, you may delete it twice (as HideSplashScreen can be called more often than once).

All these versions set the pointer to null after a delete (except the first one above, but only if it was already null).

Quote
Deleting the same object twice is a MCA.

MCA?

Since HideSplashScreen is usually called exactly once, and in the exceptional case twice, I believe it really does not pay to spend time even thinking about one more pointer assignment :lol:

Quote from: Urxae on January 24, 2006, 09:33:16 PM
MCA?

MCA = GAU

Quotevoid CodeBlocksApp::HideSplashScreen()
{
    delete m_pSplash;
    m_pSplash = 0;
}

That's it : deleting NULL pointers is safe in C++, see one of the Herb Sutter books.

C++ guarantees that operator delete checks its argument for null-ness. If the argument is 0, the delete expression has no effect. In other words, deleting a null pointer is a safe (yet useless) operation. There is no need to check the pointer for null-ness before passing it to delete.

Michael

Michael, I think we have a lift off  :P

Well, if you really think that this will make anything better, then you should at least do it right, there are a couple more of these constructs... :)

Like so:

[attachment deleted by admin]

Quote from: thomas on January 24, 2006, 10:54:18 PM
Well, if you really think that this will make anything better, then you should at least do it right, there are a couple more of these constructs... :)

Like so:
[attachment: useless.patch.txt]

Actually, that looks like a nice code cleanup¹, which I wouldn't call useless at all. :D

1) Especially with the annoying new wxCriticalSectionLocker removal you seem to have thrown in; what's the point of wxCriticalSectionLocker if you're going to dynamically allocate it? :shock:

Quote from: Urxae on January 25, 2006, 12:03:32 AM1) Especially with the annoying new wxCriticalSectionLocker removal you seem to have thrown in; what's the point of wxCriticalSectionLocker if you're going to dynamically allocate it? :shock:

I had already made that particular modification 2 months ago, but it somehow made its way out again...
My point with an automatic critical section locker inside a curly brace block is that it is about 200 times faster than dynamically allocating and manually freeing it, and it is exception-safe too, which is the main reason why you use a locker object in the first place. :)

Quote from: thomas on January 25, 2006, 12:30:28 AM

Quote from: Urxae on January 25, 2006, 12:03:32 AM1) Especially with the annoying new wxCriticalSectionLocker removal you seem to have thrown in; what's the point of wxCriticalSectionLocker if you're going to dynamically allocate it? :shock:

[...]
My point with an automatic critical section locker inside a curly brace block is that it is about 200 times faster than dynamically allocating and manually freeing it, and it is exception-safe too, which is the main reason why you use a locker object in the first place. :)

You realize that was a rhetorical question, right?

EDIT: Oh, and in addition to the reasons you gave, you can always call wxCriticalSection::Enter() and wxCriticalSection::Leave() instead of dynamically creating and destroying a locker, for the exact same effect but without the dynamic allocation overhead. (So still not exception-safe, but at least as fast as a non-dynamically allocated locker)
(Therefore there is absolutely no reason to ever use new wxCriticalSectionLocker. There is always a better way)