Sign in to follow this  
CirdanValen

Where did my list go?

Recommended Posts

EDIT: Turns out I WAS trying to delete it twice >.< Tho, I find it strange the breakpoint in the deconstructor only triggered the one time the deconstructor failed

I have a std::list of Controls for my UI, which is initialized like this:

[code] Button *newGame = new Button("newgame", "New Game", this);
newGame->setPosition(Vector<int>(100, 300));

Button *exitGame = new Button("exitgame", "Exit", this);
exitGame->setPosition(Vector<int>(100, 340));

mUIManager->addControl(newGame);
mUIManager->addControl(exitGame);[/code]

In the UIManager's deconstructor, I have this:

[code]UIManager::~UIManager() {
if(!mControls.empty()) {
for(std::list<Control*>::iterator itr = mControls.begin(); itr != mControls.end();) {
Control *c = (*itr);
itr = mControls.erase(itr);
delete c;
}
}

if(mFocusedControl) {
delete mFocusedControl;
}
}[/code]

Now, for some reason, when it comes time to clean up the list of controls, the list doesn't exist and the program crashes when trying to access mControls...even for just checking if it's empty. Walking through it with the debugger shows that the deconstructor is only being called once (if its even possible to call it more than once?)

Here is the full UIManager

[source]UIManager::UIManager() {
mFocusedControl = NULL;
}

UIManager::~UIManager() {
if(!mControls.empty()) {
for(std::list<Control*>::iterator itr = mControls.begin(); itr != mControls.end();) {
Control *c = (*itr);
itr = mControls.erase(itr);
delete c;
}
}

if(mFocusedControl) {
delete mFocusedControl;
}
}

void UIManager::handleInput(SDL_Event &e) {

if(e.type == SDL_MOUSEMOTION) {
if(mFocusedControl && !mFocusedControl->getBounds().containsPoint(e.motion.x, e.motion.y)) {
mFocusedControl->onMouseLeave(e);
mFocusedControl = NULL;
}

for(std::list<Control*>::iterator itr = mControls.begin(); itr != mControls.end(); itr++) {
Control *c = (*itr);
if(c->getBounds().containsPoint(e.motion.x, e.motion.y) && !mFocusedControl) {
c->onMouseEnter(e);
mFocusedControl = c;
break;
}
}
}

if(e.type == SDL_MOUSEBUTTONDOWN && mFocusedControl) {
mFocusedControl->onMousePressed(e);
}

if(e.type == SDL_MOUSEBUTTONUP && mFocusedControl) {
mFocusedControl->onMouseReleased(e);
}

if(e.type == SDL_KEYDOWN && mFocusedControl) {
mFocusedControl->onKeyPressed(e);
}

if(e.type == SDL_KEYUP && mFocusedControl) {
mFocusedControl->onKeyReleased(e);
}
}

void UIManager::addControl(Control *control) {
mControls.push_back(control);
}

void UIManager::tick(Graphics *g, int deltaTime) {
for(std::list<Control*>::iterator itr = mControls.begin(); itr != mControls.end(); itr++) {
(*itr)->tick(g, deltaTime);
}
}[/source]

Share this post


Link to post
Share on other sites
Welcome to the forums! First some helpful notes to get you started:
Don't check a pointer for null before calling delete on it. That exact check is already done inside delete, you're only causing it to have to check twice.
Don't check if a container is empty before walking it. The very first instruction executed for the loop is to test if begin() is equal to end(), which it will be if the container is empty. So again all it does is amount to checking the same thing twice.
It isn't necessary to remove the items in a container as you go. In fact for any other container type it is a lot slower. For a list it's more or less the same, but still unnecessary.
I would thus shorten your destructor to just this:
[code]UIManager::~UIManager() {
for (std::list<Control*>::const_iterator itr = mControls.begin(); itr != mControls.end(); ++iter)
delete *iter;
delete mFocusedControl;
}[/code]Less code means less opportunity for bugs.
However, I hope you're already ensuring that mFocusedControl does not point to one of the items in the list, as you'd then have a double-delete there. If that's not your immediate problem, then my guess is that you do not know about, and hence have not followed, [url="http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)"]The Rule of Three[/url].

Whilst the result is often the same, this is not the preferred way to initialise a class member in C++:
[code]UIManager::UIManager() {
mFocusedControl = NULL;
}[/code]
[font="Arial"]This is:[/font] [code]UIManager::UIManager() : mFocusedControl(NULL) {}[/code]It is called a constructor initialisation list, and tends to be more efficient, or where references are used it is actually the only way to do it.

The next thing, which others will likely elaborate on, is that it is highly preferable to either not store raw pointers in a container, or to use a boost ptr_container.

FYI, it's "destructor" not "deconstructor". You're destroying the object, not merely taking it apart.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this