Re: [Flightgear-devel] Re: SGPropertyNode::removeChildren()
Erik Hofman a écrit : Andy Ross wrote: Seriously: the removeChild() method is just buggy. It should never have cared about refcounting at all. Andy, I have to agree with Melchior here. If you call removeChild you have the intention that it will stay in the tree until refcount becomes zero and then it will be deleted. If you call removeChild() and it just detached from the tree (without cleaning it up at some point) you won't even be allowed to access it using the property tree, even after the first call to removeChild(). I am rather on Andy's line here. As I tried to explain to Melchior 'remove' doesn't mean 'destroy'. I think the only reason the tree wasn't actually deleted/destroyed when the reference returned by removeChild was deleted is that it was copied in the _removedChild vector before returning, hence incrementing the counter. If you don't care having aliases pointing to garbage ( something David was against ), you would just have to dump the _removedChild vector and, if the SGPropertyNode_ptr returned is not used, the tree should be destroyed. Two functions shouldn't be necessary. -Fred ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
[Flightgear-devel] Re: SGPropertyNode::removeChildren()
* Frederic Bouvier -- Thursday 30 June 2005 08:51: I am rather on Andy's line here. As I tried to explain to Melchior 'remove' doesn't mean 'destroy'. We know. It was said a few times in this thread already. removeChild() just *pretended* to remove something, when it actually only detached it. If the scope of the returned guarded pointer ended, the whole subtree basically ended as zombie (depending on the keep option). Nobody in fgfs knew this -- each of the four places that used it, intended to really remove the whole subtree. Only fgsd, for which it was written, knew the secret. (Don't expect all people to dive into the sg code!). There was no way to really remove a subtree -- the only function that pretended it would, failed at it badly. All the consequence of a poorly chosen name. it was copied in the _removedChild vector before returning, hence incrementing the counter. If you don't care having aliases pointing to garbage ( something David was against ), you would just have to dump the _removedChild vector and, if the SGPropertyNode_ptr returned is not used, the tree should be destroyed. Two functions shouldn't be necessary. It would have to do this recursively. Because neither SGPropertyNode_ptr nor SGPropertryNode do AFAIK destroy their children on destruction. (And I don't know if this would be desirable.) So there *had* another function to be written. Andy had suggested to remove the whole _removed_children thing[1]. So what is it now? Just pretend everything was well, when it wasn't? The old system was misleading and broken, and we discuss if the new is broken, too. Fine. Then fix either. But whatever the decision is: the current method doesn't mess with the counter. It doesn't increase it, and it doesn't decrease it[2], nor does it change existing conditional behavior of other functions based on the counter. Everything is as it used to be -- the zombie thing still available as detachChild(). We are in fact discussing if the newly added removeChild is allowed to make decisions based on the refcounter. That's all. If it may or may not refuse to remove nodes from the main tree if they are still in use, and referenced by anyone else than just the parent. I thought it would be a good idea to do it like that, but I don't care much if it's changed to not consider the refcounter at all. I just don't want to see the zombie generator used under the misleading name removeChild() again. m. [1] http://baron.flightgear.org/pipermail/flightgear-devel/2005-May/036227.html [2] OK, alias and unalias do that now, but could be changed to make use of a SGPropertyNode_ptr. Fact is that they need to affect the refcounter somehow. ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
Re: [Flightgear-devel] Re: SGPropertyNode::removeChildren()
Andy Ross wrote: If you guys need or want a bit that says leave in tree until condition X has occurred, then that's fine. But you can't use the refcount to do it, that's not what it's for. Reference counting is used to make sure that (a) memory is never freed when there are live pointers to it, and (b) live pointers never point to freed memory. Any other usage* is just wrong, and causes bugs**. * In this case: using the reference count to tell whether or not the node is in the tree or detached. I'll have to take a look at the code to make sure, but as far as I know this is exactly what is happening. You don't have to be pedantic, I know what it's meant for; Counting the number of references, and only act when the counter reaches zero. That is what the current code should do or am I missing something here? I'll take a look at it. Erik ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
Re: [Flightgear-devel] Re: SGPropertyNode::removeChildren()
Melchior FRANZ wrote: * Frederic Bouvier -- Thursday 30 June 2005 08:51: I am rather on Andy's line here. As I tried to explain to Melchior 'remove' doesn't mean 'destroy'. We know. It was said a few times in this thread already. removeChild() just *pretended* to remove something, when it actually only detached it. Ok, since no one cares to describe the difference to me in Dutch or German, can we all agree on the following: Destroy: erase it's exsistance immidiately, no matter what (we don't even care about aliases pointing to nowhere) Remove: tell the library the node is of no use anymore, let the library decide if or when it should actually be erased from memory (it could also be called Delete, but it isn't.) Detach: make sure the node can't be referenced from the original tree anymore, it becomes it's own root node. Let the library decide of or when it should be erased from memory. Erik ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
[Flightgear-devel] Re: SGPropertyNode::removeChildren()
* Melchior FRANZ -- Thursday 30 June 2005 09:40: If the scope of the returned guarded pointer ended, the whole subtree basically ended as zombie [...] It would have to do this recursively. Because neither SGPropertyNode_ptr nor SGPropertryNode do AFAIK destroy their children on destruction. (And I don't know if this would be desirable.) So there *had* another function to be written. Except that they *do* destroy their children with the same refcount-guarded delete mechanism, right? So the main argument against the new method shouldn't have been it casts a shadow on the wonderful world of refcounting, but the problem that you are trying to solve doesn't even exist. :-) So we'll revert all these patches and return to the original state, plus: refcounting for aliases(?), and maybe an added removeChildren()? So much work for nothing ... sigh ... m. PS: :-P ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
Re: [Flightgear-devel] Re: SGPropertyNode::removeChildren()
Erik Hofman a écrit : Melchior FRANZ wrote: * Frederic Bouvier -- Thursday 30 June 2005 08:51: I am rather on Andy's line here. As I tried to explain to Melchior 'remove' doesn't mean 'destroy'. We know. It was said a few times in this thread already. removeChild() just *pretended* to remove something, when it actually only detached it. Ok, since no one cares to describe the difference to me in Dutch or German, can we all agree on the following: Destroy: erase it's exsistance immidiately, no matter what (we don't even care about aliases pointing to nowhere) Remove: tell the library the node is of no use anymore, let the library decide if or when it should actually be erased from memory (it could also be called Delete, but it isn't.) Detach: make sure the node can't be referenced from the original tree anymore, it becomes it's own root node. Let the library decide of or when it should be erased from memory. At the time of implementing the functionality ( in may 2002, unfortunately the CVS repository was reset in September 2002 ), remove was meant as 'remove from tree' ( 'detach' in your proposed phraseology ) not 'remove from memory'. The smart pointer should have decided if the subtree removed from the tree should have been destroyed or not based on the reference counter. Because of the _removedChildren vector, no destroy was actually done. -Fred ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
[Flightgear-devel] Re: SGPropertyNode::removeChildren()
* Frederic Bouvier -- Thursday 30 June 2005 15:34: For a reason : it is the job of SGPropertyNode_ptr destructor : [...] If the vector of children SGPropertyNode_ptr is properly cleared in the SGPropertyNode destructor, the whole tree should go away recursively. Meanwhile I know that, too. If only you had told me in one of the nine private messages about this issue. You didn't know it exactly any more, either? Well, nobody seems to have. Except probably David ... m. ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
[Flightgear-devel] Re: SGPropertyNode::removeChildren()
* Frederic Bouvier -- Thursday 30 June 2005 16:05: Melchior FRANZ a écrit : I am sorry about that, and please accept my apologies. I didn't get into the issue until you change the API, and I only understood you didn't get the idea until your last message. Heh ... and I apologize to all for the false alarm and the needless inconvenience, especially to Erik for dragging him into that and (successfully :-) fooling him. I've learned a lot about the property system now, maybe it'll at least pay in the future. But reverting doesn't solve all problems (although I could again be wrong): alias() and unalias() referenced pointers *without* increasing/decreasing the refcounter. So they could lose their target and crash. I simply let them call incrementRef/decrementRef on their target. This should then be done with SGPropertyNode_ptr, too, right? And a removeChildren() (derived from the old removeChild(), not a clever new design based on wrong assumptions :-) would also be handy. And all callers of removeChild() should set keep to false. Only one did so far. Mine didn't, because the documentation left me quite unclear about why I would or would not want that. m. ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
Re: [Flightgear-devel] Re: SGPropertyNode::removeChildren()
Melchior FRANZ wrote :
* Frederic Bouvier -- Thursday 30 June 2005 16:05:
Melchior FRANZ a écrit :
I am sorry about that, and please accept my apologies. I didn't get into
the issue until you change the API, and I only understood you didn't get
the idea until your last message.
Heh ... and I apologize to all for the false alarm and the needless
inconvenience, especially to Erik for dragging him into that and
(successfully :-) fooling him. I've learned a lot about the property
system now, maybe it'll at least pay in the future.
But reverting doesn't solve all problems (although I could again be wrong):
alias() and unalias() referenced pointers *without* increasing/decreasing
the refcounter. So they could lose their target and crash. I simply let
them call incrementRef/decrementRef on their target. This should then be
done with SGPropertyNode_ptr, too, right?
What about :
// The right kind of pointer...
union {
-SGPropertyNode * alias;
+SGPropertyNode_ptr alias;
SGRawValuebool * bool_val;
SGRawValueint * int_val;
I don't remember why it wasn't done that way at the first time.
And a removeChildren() (derived from the old removeChild(), not a clever
new design based on wrong assumptions :-) would also be handy.
Sure
And all callers of removeChild() should set keep to false. Only one
did so far. Mine didn't, because the documentation left me quite unclear
about why I would or would not want that.
I am not sure about the 'keep' thing. Is there any reason duplicating
the same thing. If you want to keep a subtree, hold the reference
yourself and don't destroy the returned SGPropertyNode_ptr, and dump
_removedChildren. I am afraid the alias feature is complicating the
whole design.
-Fred
___
Flightgear-devel mailing list
Flightgear-devel@flightgear.org
http://mail.flightgear.org/mailman/listinfo/flightgear-devel
2f585eeea02e2c79d7b1d8c4963bae2d
[Flightgear-devel] Re: SGPropertyNode::removeChildren()
[cc'd to flightgear-devel, as this is clearly an architectural issue and not a bug report.] The basic issue as I understand it is that apparently SGPropertyNode::removeChild() has been (incorrectly, AFAICT) modified to ignore nodes that have a reference count greater than one, which interacts badly with the case where a Nasal script has created a Node object (which includes a refcounted pointer, obviously) that has yet to be garbage-collected. Melchior FRANZ wrote: removeChild() doesn't remove nodes with refcount != 1, nor their children (independent of their counter). After destroying the SGPropertyNode_ptr you return to count == 1 (i.e. only referenced by parent). Same state as before calling removeChild(). Nothing will be deleted by that. Why not? This sounds like a bug to me. What is wrong with removing a node that someone else is holding a reference to? When they free their reference, then it gets deleted. No. removeChild/ren() is correct. Just don't take guarded pointers from nodes that you want to remove. That's not possible in Nasal. You create the refcount in getNode(), and have no way to know whether the script intends to delete it or not. Let me put it more forcefully: if proper reference counting semantics are going to be removed from the property system, then the current Nasal props.Node interface WILL NOT WORK, and all code will have to be ported to the stateless getprop()/setprop() interface instead. Seriously: what is the bug being fixed by the ignoring the refcount thing? The whole idea behind reference counting is to *remove* the requirement for this kind of logic. Andy ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
[Flightgear-devel] Re: SGPropertyNode::removeChildren()
* Andy Ross -- Wednesday 29 June 2005 19:49: The basic issue as I understand it is that apparently SGPropertyNode::removeChild() has been (incorrectly, AFAICT) modified No. The only modification was a name change. It's now called detachChild(). A new function has been added that does really remove a node with all children. You can still use detachChild() and get exactly the same buggy behavior as before. What is wrong with removing a node that someone else is holding a reference to? Well, the theory is that the property tree is there to inspect/access values. And the idea is that a remove function shouldn't only detach property nodes that are then secretly used behind the users back. If you want them to stay, don't remove them. If you want to remove them, don't tie a reference to them. I see your point, but you don't see mine. Should there be an option bool really = true? The current behavior is (if I understood Fred correctly) the way David had planned. The old removeChild() was just a (misnamed) solution for a different demand. When they free their reference, then it gets deleted. But it doesn't. That's the whole point. The SGPropertyNode_ptr destructor doesn't remove the node's children. Of course, it would have been possible to let it do that. But see above. No. removeChild/ren() is correct. Just don't take guarded pointers from nodes that you want to remove. That's not possible in Nasal. OK. So we are actually talking about a Nasal problem? :-} m. ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
Re: [Flightgear-devel] Re: SGPropertyNode::removeChildren()
Melchior FRANZ wrote: Well, the theory is that the property tree is there to inspect/access values. And the idea is that a remove function shouldn't only detach property nodes that are then secretly used behind the users back. If you want them to stay, don't remove them. Reference counting isn't about removal or detachment. It is a low level API convention that deals with *allocation*. This argument seems to be confusing the two. In this particular case, no one wants [the node] to stay. But some parts of the code may need to hold a *pointer* to the node in a place that is not accessible to the code making the removeChild() call. So you decrement the refcount instead during detachment (not destruction!), and the *library* decides when to destroy (not detach!) the object. Again: destroy (low level C++ concept) is not the same thing as remove (high level property concept). Think about it this way and things should be clearer. When they free their reference, then it gets deleted. But it doesn't. That's the whole point. The SGPropertyNode_ptr destructor doesn't remove the node's children. Again, the disconnect: I'm talking about deletion (i.e. whether the memory is freed or leaked), and you are talking about logical property stuff like where the node appears in the tree. These aren't the same concept. It might not be desirable to have SGPropertyNode objects that are detached from the property but still be live in the sense that reachable pointers can find them. You might hate this concept. It might be bad design. BUT IT MUST BE LEGAL! Otherwise you cannot have garbage collected property references. Otherwise you cannot have automatic deletion across C++ exceptions. Otherwise you cannot put a property node pointer on the stack at all and expect it not to leak. Otherwise you get NO VALUE WHATSOEVER from the nice refcounting implementation that David Megginson gave us. That's not possible in Nasal. OK. So we are actually talking about a Nasal problem? :-} It's not possible. It can't be fixed. You can blame me personally if you want, but nothing will make the current removeChild() implementation compatible with garbage collected Nasal nodes. The latter depends on the reference counting semantics*, while the former depends on a special case side effect** of our implementation. These are fundamentally incompatible metaphors. Pick one to eject, you can't have them both. * A standard concept that David Megginson implemented in the standard way; I had no trouble understanding what a SGPropertyNode_ptr was or how to use it, for example. As a result, Nasal has access to leak-proof and garbage-proof handles to property nodes (something that would basically be impossible without the refcount mechanism or something like it). This, I argue, is a feature and not a bug. ** Specifically, it assumes that the parent/child relationship in the property try is the *only* context in which the reference counting will be used, which is silly and breaks the really nice features that refcounting gives you. It is also false; if that were the case SGPropertyNode_ptr would not be a public API. Note all the other places that use it. Seriously: the removeChild() method is just buggy. It should never have cared about refcounting at all. Andy ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
[Flightgear-devel] Re: SGPropertyNode::removeChildren()
This can be changed by just removing two checks. And it's still far less buggy than the old version ... that you can still use (I can send you a patch that replaces removeChild with detachChild. ;-) I'm happy with everything except reverting to the zombie generator. m. ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
[Flightgear-devel] Re: SGPropertyNode::removeChildren()
* Melchior FRANZ -- Wednesday 29 June 2005 21:48: it's still far less buggy than the old version ... that you can still use (I can send you a patch that replaces removeChild with detachChild. ;-) This is not entirely fair. The old version wasn't buggy. It did correctly what it wanted to do. It just didn't do what it implied it would do, at least not what 100% of its users in fgfs thought it would do. It left the whole removed subtree in memory, only detached from the tree. That's not what we want when we throw away all the unused joystick driver information. m. ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
Re: [Flightgear-devel] Re: SGPropertyNode::removeChildren()
Andy Ross wrote: Seriously: the removeChild() method is just buggy. It should never have cared about refcounting at all. Andy, I have to agree with Melchior here. If you call removeChild you have the intention that it will stay in the tree until refcount becomes zero and then it will be deleted. If you call removeChild() and it just detached from the tree (without cleaning it up at some point) you won't even be allowed to access it using the property tree, even after the first call to removeChild(). It's just a matter of properly naming functions anyhow, and therefore the old behavior is available under a different function call: detachChild(). It makes clear to everybody who uses it that it actually detached a node from the tree, but not removes it from memory when calling it. If Nasal relies on that behavior it's just as easy as calling the proper function again and all should be set. If that's not the case then detachChild() should be modified, but as far as I know it does exactly what you are asking for. Erik ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
Re: [Flightgear-devel] Re: SGPropertyNode::removeChildren()
Erik Hofman wrote: Andy, I have to agree with Melchior here. If you call removeChild you have the intention that it will stay in the tree until refcount becomes zero and then it will be deleted. If you call removeChild() and it just detached from the tree (without cleaning it up at some point) you won't even be allowed to access it using the property tree, even after the first call to removeChild(). I have no objection to either of these scenarios. My objection is improperly overloading the reference count (which has *nothing* to do with trees or property values at all!) to implement it. If you guys need or want a bit that says leave in tree until condition X has occurred, then that's fine. But you can't use the refcount to do it, that's not what it's for. Reference counting is used to make sure that (a) memory is never freed when there are live pointers to it, and (b) live pointers never point to freed memory. Any other usage* is just wrong, and causes bugs**. * In this case: using the reference count to tell whether or not the node is in the tree or detached. ** In this case: Nasal can keep a refcounted pointer for a short time until it gets garbage collected and deleted, which make the node undetachable until the collector runs. Just call it something else, and leave the refcount value alone. What you guys are doing is not reference counted allocation. Andy ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
[Flightgear-devel] Re: SGPropertyNode::removeChildren()
* Andy Ross -- Wednesday 29 June 2005 23:42: Reference counting is used to make sure that (a) memory is never freed when there are live pointers to it, and (b) live pointers never point to freed memory. Any other usage* is just wrong, and causes bugs**. * In this case: using the reference count to tell whether or not the node is in the tree or detached. That's not what it's used for. Actually, it prevents nodes from disappearing from our property node memory representation -- which is the property tree. The old method was broken, and reverting to it is out of the question. Fixing the new one is, of course, desirable *if* it's broken. m. ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
Re: [Flightgear-devel] Re: SGPropertyNode::removeChildren()
Melchior FRANZ wrote: Andy Ross wrote: * In this case: using the reference count to tell whether or not the node is in the tree or detached. Actually, it prevents nodes from disappearing from our property node memory representation -- which is the property tree. It sounds to me like these are saying exactly the same thing. Guys, please: The reference count is a count of pointers to an object. Nothing more. Making it mean anything else is *not* a reference count. Reference counting is a well-established idiom. Plib uses it. The auto_ptr uses it. It apperas in pretty much every textbook on C++. When I saw that the David's SGPropertyNode class implemented a refcounted pointer, I knew *exactly* how to use it, because it worked like all the other ones I have seen. And guess what? It worked. The new reference count semantics are not the same as the ones plib uses. They are not the same as auto_ptr, or the treatments in textbooks. And therefore they are confusing, and cause bugs. Changing the meanings of well-established idioms in critical infrastructure like the property system is just really bad design. Sorry, but it is. If you insist on re-purposing the reference count, then please rename it to something else (that is, after you've removed the props.Node interface from Nasal). Fixing the new one is, of course, desirable *if* it's broken. Huh? This whole discussion started when you reported that after the new changes, removing property nodes from Nasal was broken. The reason is that the change broke the refcount semantics that Nasal is relying on. Andy ___ Flightgear-devel mailing list Flightgear-devel@flightgear.org http://mail.flightgear.org/mailman/listinfo/flightgear-devel 2f585eeea02e2c79d7b1d8c4963bae2d
