24 replies to this topic

[kyuzo]

hi all, i'm new here.

here's my problem:
i have a GUI library based on windows; all the widgets inherits from a base class; this class has a list of child widgets (a std::vector, actualy), and its destructor also deletes all the widgets children, like this:

if(parent){//remove this kid from the parent's list
std::vector<WinObject *>::iterator it;
for(it=parent->children.begin();it!=parent->children.end();it++)
if((*it)==this){
parent->children.erase(it);
break;
}
}
std::vector<WinObject *>::iterator it=children.begin();
while(children.size()){
delete(*it);
}

While this works perfectly in vc6 and in mingw (gcc 3.4.2) it fails in vc2005 express edition, with a "vector iterator not dereferencable" error. Now, i have no idea, it is vc2005, it is my code?
Thank you for any help.

[mysticman]

hi Kyuzo! (also I am Italian)

The problem I don't know whether to resolve it, but I have had to abdicate to a project c++ with the vc2005pro and to bring it on vc2003. Too many problems, especially if it has some functions that ask for the directx SDK....
sorry for my bad engrish! :blush:

[kyuzo]

most probably is my code, not vc2005; i'll come back with the solution, if i'll find it...hopefuly.

[Axel]

Is that a compile- or runtime error?

std::vector<WinObject *>::iterator it=children.begin();
while(children.size()) delete(*it);

I don't even understand why this doesn't generate an infinite loop. The size of the children vector will not decrease if you delete the objects that it points to.

Edit: You don't even increment the iterator in the loop. WTF?

[kyuzo]

that's a runtime error.
it doesnt generate an infinite loop, as there's a parent->children.erase(it); before the code you've quoted.

[Reedbeta]

No, Axel is right. The original code, indented for more clarity, is

if(parent){//remove this kid from the parent's list
	std::vector<WinObject *>::iterator it;
	for(it=parent->children.begin();it!=parent->children.end();it++)
		if((*it)==this){
			parent->children.erase(it);
			break;
		}
}
std::vector<WinObject *>::iterator it=children.begin();
while(children.size()){
	delete(*it);
}

First of all, the loop that tries to remove 'this' from 'parent' isn't guaranteed to work properly because the iterator is invalidated by the call to erase(). And as Axel says, the second loop absolutely doesn't do what it is supposed to. What you probably want is like this:

if (parent)
{
    iterator i = parent->children.begin();
    while (i != parent->children.end())
        if (*i == this) i = parent->children.erase(i); else ++i;
}
iterator i = children.begin(), end = children.end();
for (; i != end; ++i)
    delete *i;

Note that erase() invalidates the iterator passed into it, and in a vector it also invalidates all the iterators after it, including end() (and potentially all iterators period, if a reallocation to smaller size occurs). With a list, this wouldn't be a problem. In either case, it returns a valid iterator to the position after the deleted item.

[kyuzo]

i thought i was decent in c++, but i was wrong. cant make it work.
here a small program that illustrates my problem:

#include<list>



class Base{

	std::list<Base *> children;

	Base *parent;

public:

	Base(Base *p=0);

	~Base();

};


Base::Base(Base *p):parent(p){

	if(p)

		p->children.push_back(this);

}


Base::~Base(){

	std::list<Base *>::iterator i,end;

	if (parent){

    i = parent->children.begin();

    while (i != parent->children.end())

        if (*i == this) i = parent->children.erase(i); else ++i;

	}

	i = children.begin(), end = children.end();

	for (; i != end; ++i)

    delete *i;

}


int main(){

	Base *window, *menu;

	window=new Base();

	menu=new Base(window);

	delete window;

}

the destructor should clean all his children, it does not.
i'd appreciate any help you can give (again)

[Reedbeta]

That destructor code definitely should call the destructor on each child and reclaims the memory allocated by the child.

It does NOT clear the vector, so at the end of the destructor it will still contain a list of (no longer valid) pointers to children. But the vector's destructor will take care of this, you don't have to do it yourself.

Also, if you are inheriting anything from Base, you need to make its destructor virtual. Otherwise the derived class's destructor will not be called.

[kyuzo]

do you mean that this code is working for you!? it gaves me a "list iterator not incrementable" error at runtime.

[kyuzo]

just to make things clear, suppose the program is like:

int main(){

	Base *window, *menu,*menitem;

	window=new Base();

	menu=new Base(window);

	menuitem=new Base(menu);

        delete menuitem;

	delete window;

}

for the earlier version, a destructor like

Base::~Base(){

  for(i=children.begin();i!=children.end();++i)

    delete *i;

}

would do.

thanks.

[Reedbeta]

Okay, I've looked at it more closely and I see what's happening. The call to 'delete *i' causes the child's destructor to run, and the child removes itself from the parent's list. This invalidates the iterator in the parent's destructor, and when control returns there the system chokes.

Is there any situation in which you would want to remove a control from its parent without deleting the parent? If not, I suggest just using the destructor you posted above. It will recursively delete all children upon deletion of the top-level control. So there would be no need to call 'delete menuitem', as 'delete window' will delete all the controls in the correct order.

Otherwise, I suggest creating a new member function whose purpose is to remove a child from a control. You could tell this member not to do anything if it was called while destructing, e.g. by using a private flag.

Another approach might be to set the child pointer in the list to NULL when the child is deleted. But then whenever you looped over the children you would have to check to be sure you skipped over null pointers.

[kyuzo]

Reedbeta said:

Is there any situation in which you would want to remove a control from its parent without deleting the parent?

The idea was to make this library work like a GUI builder too, like when i #define DESIGN, i can drag&drop, modify, delete the widgets. It kind of worked, then i thought to switch to vc++2005 and it stopped working. Anyway, thank you all for your help, i will try again.

[Reedbeta]

Well, I hope you get it working. Just to let you know, I think the code was incorrect from the beginning; it was just pure chance that it happened to work right under VC6.

[kyuzo]

ok, here's what i have:

std::vector<WinObject *>::iterator it;

if(parent){

	it=parent->children.begin();

	while(it!=parent->children.end())

		if(this==*it){

			parent->children.erase(it);

			break;

		}

}

while(children.size()){

	it=children.begin();

	delete *it;

}

@Reedbeta:
i know you dont like it (though i cant see any obvious error), but all the children widgets get deleted, it works in both vc6 and vc2005, so i guess i leave it as it is right now, at least for the moment.

[Reedbeta]

kyuzo,

That may work when you are deleting the parent, so it deletes all its children in order, but it won't work if you want to delete a single arbitrary child (like in your GUI-builder idea). You're not incrementing the iterator in the first loop, so it will only find 'this' in parent->children if 'this' is the first element!

[kyuzo]

oh, yes, thanks for noticing it! it needs an "else ++it;"
anyway, it looks that puting the "it=children.begin();" inside the while loop got the job done. dont ask me why.

[.oisyn]

Also, you should better use empty() to test whether a container is empty or not, as size() is not guaranteed to be a constant time function (while empty() is)

[Faelenor]

VC 2005 is a lot more standard than the previous versions. If something doesn't work in 2005 and was working in 2003, then you know that you were doing something wrong. The STL implementation in 2003 was really bad.

kyuzo: maybe a book about stl would be useful... I suggest Effective STL.

[CobraLionz]

This code does not compute.

kyuzo said:

ok, here's what i have:

std::vector<WinObject *>::iterator it;

if(parent){

	it=parent->children.begin();

	while(it!=parent->children.end())

		if(this==*it){

			parent->children.erase(it);

			break;

		}

}

while(children.size()){

	it=children.begin();

	delete *it;

}

@Reedbeta:
i know you dont like it (though i cant see any obvious error), but all the children widgets get deleted, it works in both vc6 and vc2005, so i guess i leave it as it is right now, at least for the moment.

[kyuzo]

uh, what do you mean? it works for me (add an ++it in the 1st loop, of course).