Code::Blocks Forums

Developer forums (C::B DEVELOPMENT STRICTLY!) => Development => Topic started by: sethjackson on January 24, 2006, 09:21:31 pm

Title: app.cpp
Post by: sethjackson on January 24, 2006, 09:21:31 pm
Hi got a tiny question about app.cpp.

Why this?

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

I belive it should be this....

Code: cpp
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]
Title: Re: app.cpp
Post by: 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).

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.
Title: Re: app.cpp
Post by: Urxae on January 24, 2006, 09:33:16 pm
Actually, it's perfectly OK to delete a null pointer. So maybe it should be
Code: cpp
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
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?
Title: Re: app.cpp
Post by: thomas on January 24, 2006, 09:37:22 pm
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
Title: Re: app.cpp
Post by: killerbot on January 24, 2006, 09:37:49 pm
Quote
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.

Title: Re: app.cpp
Post by: Michael on January 24, 2006, 10:13:13 pm
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
Title: Re: app.cpp
Post by: killerbot on January 24, 2006, 10:47:48 pm
Michael, I think we have a lift off  :P
Title: Re: app.cpp
Post by: 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 deleted by admin]
Title: Re: app.cpp
Post by: Urxae on January 25, 2006, 12:03:32 am
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 cleanup1, 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:
Title: Re: app.cpp
Post by: thomas on January 25, 2006, 12:30:28 am
Quote from: Urxae on January 25, 2006, 12:03:32 am
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. :)
Title: Re: app.cpp
Post by: Urxae on January 25, 2006, 01:15:42 am
Quote from: thomas on January 25, 2006, 12:30:28 am
Quote from: Urxae on January 25, 2006, 12:03:32 am
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)