app.cpp

[sethjackson]

Hi got a tiny question about app.cpp.

Why this?

Code (cpp) Select Expand


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


I belive it should be this....

Code (cpp) Select Expand


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]

[thomas]

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.

[Urxae]

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

Code (cpp) Select Expand


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

Code (cpp) Select Expand


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

is probably even better.

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).

Deleting the same object twice is a MCA.

MCA?

[thomas]

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:

MCA?

MCA = GAU

[killerbot]

void 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.

[Michael]

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

[killerbot]

Michael, I think we have a lift off 

[thomas]

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]

[Urxae]

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.

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:

[thomas]

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:

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.

[Urxae]

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:

[...]
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)