| Hi Fred,
Thanks for reviewing this code! On Feb 23, 2012, at 2:37 AM, Fred Kiefer wrote:
What I was looking for was a specific example that shows the need for the current retain count handling. We can only touch this, if we know about the problems we try to solve here. I don't like the current solution too much, as it wont work with GC or ARC, but wont touch it until we have enough test cases that show that an alternative solution is actually working.
First, let me assure you that we have code in our project that does exercise the current retain count mechanism and appears to be working properly. I agree that it would be good to add some GNUstep tests to work with this as well. One type of scenario that should be tested would be to create an NSXMLDocument from an XML string. This will create only the top-level object. Then access a node farther down the tree, either by calling childAtIndex: a couple of times to get a grandchild, or alternatively by using an XPath call to go directly to some descendant node (or better yet, test both scenarios). The code will automatically create Objective-C objects for the nodes that are accessed as well as each node in the parent chain leading from that node up to the document.
After obtaining a lower-level node object, release the document object. None of the ObjC objects that were created should be dealloc'ed as long as there is a retain on any of them from somewhere outside of the tree. Upon releasing the lower-level node, all of the objects in the tree should be dealloc'ed and the underlying libxml2 tree structure will be freed. To test this thoroughly may require wrapping the test in an autorelease pool. While cleaning up the code I found a few issues that I would like to discuss here. One is the isEqual: method on NSXMLNode, the current implementation also checks the siblings of the node, which is obviously wrong and easy to solve (Just iterate over all children). But I would like to change the whole implementation here. Instead of going to the libxml2 structure for the test, I would like to implement it on the Objective-C level. The benefit would be that sub-nodes can easily override the check. Any opinion on that?
I agree that isEqual: should not check the sibling nodes. We'll get that fixed. The test needs to be performed on the libxml2 structure, though, not at the ObjC level. The whole philosophy of the current implementation is that essentially all of the data resides in the libxml2 tree. ObjC objects are only created as needed, for nodes that are being accessed (plus their direct ancestors, as noted above for the memory management system). Creating ObjC objects for every node in two trees in order to test whether they are equal would be wasteful and entirely counter to this philosophy. The next thing I found is that the whole namespace handling on NSXMLElement looks wrong. The structure element "ns" isn't present in xmlElement, we only have it in xmlNode. And even there it has a completely different meaning. Also I think that namespaces are a tree concept, not just a node or element one. You always should get the aggregated namespaces of all parent nodes.
Anybody out there, with a deeper understanding of this issue? Otherwise I just start to write test cases on Cocoa and try to reimplement that behaviour.
I haven't really worked with namespaces, so I don't currently have an opinion on this although it sounds like you have made some valid points. I'll try to take a look at it later, after we wrap up our current release.
Cheers,
Doug
|