|
|
Home » U++ Library support » TreeCtrl » A [TreeCtrl] bug
A [TreeCtrl] bug [message #14549] |
Mon, 03 March 2008 07:56 |
huanghuan
Messages: 10 Registered: December 2007
|
Promising Member |
|
|
the function TreeCtrl::Find(Value) return a valid id after I remove the node.
I found the memory has managed by a Array<Item>. the remove function just push the node to the freelist by the field freelink.
So I think m.key=Null should be call in the remove function as following.
void TreeCtrl::RemoveSubtree(int id)
{
Item& m = item[id];
if(m.sel)
selectcount--;
if(m.linei == cursor)
cursor = item[m.parent].linei;
if(m.ctrl)
m.ctrl->Remove();
m.key=Null
m.value = Null;
m.image = Null;
RemoveChildren(id);
m.freelink = freelist;
freelist = id;
m.free = true;
}
|
|
|
Re: A [TreeCtrl] bug [message #14558 is a reply to message #14549] |
Mon, 03 March 2008 19:17 |
|
mirek
Messages: 13984 Registered: November 2005
|
Ultimate Member |
|
|
Thanks for the bug report!
Well, that is not bad, but IMO
int TreeCtrl::Find(Value key)
{
for(int i = 0; i < item.GetCount(); i++)
if(!item[i].free && Get(i) == key)
return i;
return -1;
}
is correct fix (well, Null key is unusual, but not impossible).
Note that Null asignments there are rather intended to release the memory....
Please check whether things work here (by replacing Find and commenting key = Null)!
Mirek
[Updated on: Mon, 03 March 2008 19:19] Report message to a moderator
|
|
|
|
Re: A [TreeCtrl] bug [message #14593 is a reply to message #14549] |
Tue, 04 March 2008 12:38 |
mr_ped
Messages: 825 Registered: November 2005 Location: Czech Republic - Praha
|
Experienced Contributor |
|
|
I didn't check TreeCtrl source nor do I know how it works, but if some memory spot is marked as "free", the value can be there undefined?
So if you find there the key you are searching for, it does not exclude the case of that particular key to be also found on some other spot, where the item[i].free == false?
In such case your optimization will break the result.
And it contains duplicate code... I would refactor it anyway to:
int TreeCtrl::Find(Value key)
{
for(int i = 0; i < item.GetCount(); i++)
if (Get(i) == key)
return item[i].free ? -1 : i;
return -1;
}
Still I think it may be broken in the case of some valid key being also stored second time in some "free" item. (But I'm too lazy to check TreeCtrl source and learn how it works and how it is used.)
Quote: | key = Null is nessary. for memory release.
|
Why? I don't see any connection between those two in C++.
Although I admit it's a good practice in C++ to always null your pointers after you released the allocated memory they are pointing to, so you don't access the freed memory by accident and rather get exception from null pointer, but actually it's "memory release for key = null", not "key = null for memory release", the reason and the consequence are the other way around.
[Updated on: Tue, 04 March 2008 12:40] Report message to a moderator
|
|
|
|
Re: A [TreeCtrl] bug [message #14595 is a reply to message #14558] |
Tue, 04 March 2008 13:16 |
mrjt
Messages: 705 Registered: March 2007 Location: London
|
Contributor |
|
|
As far as I can see Mirek's version is the correct fix here.
luzr wrote on Mon, 03 March 2008 18:17 | Note that Null asignments there are rather intended to release the memory....
|
mr_ped wrote on Tue, 04 March 2008 11:38 |
Quote: | key = Null is nessary. for memory release.
|
Why? I don't see any connection between those two in C++.
Although I admit it's a good practice in C++ to always null your pointers after you released the allocated memory they are pointing to, so you don't access the freed memory by accident and rather get exception from null pointer, but actually it's "memory release for key = null", not "key = null for memory release", the reason and the consequence are the other way around.
|
I believe this is because both key and value are Value types that may contain reference-counted objects (such as Image), and setting Null is required to decrement this count and (perhaps) free the memory. So setting Null is can be the cause of memory release.
But if this is the case then surely 'key = Null' should be added? I admit it's unlikely that a reference counted object would be used as a Key, but it seems to be cost-free and doesn't break any code so why not do it?
I could be wrong about all this though, my understanding of this aspect of Upp is somewhat limited.
James
[Updated on: Tue, 04 March 2008 13:18] Report message to a moderator
|
|
|
|
Goto Forum:
Current Time: Sat Jun 08 07:05:26 CEST 2024
Total time taken to generate the page: 0.02535 seconds
|
|
|