On Nov 11, 2010, at 15:40, Bill Cheeseman wrote: On Nov 11, 2010, at 8:42 AM, Christiaan Hofman wrote: However, you have neglected to CFRelease (or release, or autorelease) the value stored in axValue by the AXUIElementCopyAttributeValue() function call in the body of your method The Cocoa convention is that a method like your -valueForAttributeCopy: always returns an object autoreleased. So, you might want to add '[axValue autorelease]' or '[value autorelease]' before the return, or change the last line to 'return [value autorelease]', and see what happens. (If the caller of your method does releases its return value when done with it, remove that release because it would be automatically released by the autorelease added in your method).
Actually, no. The docs (Memory Management Programming Guide) say that the returned value in this case should be retained:
"You “create” an object using a method whose name begins with “alloc” or “new” or contains “copy” "
Note the word "contains", which only applies to "copy".
(I personally think this rule is wrong, or at least formulated wrongly and inconsistently in the docs. For instance compare a method initWithObjects:copyItems: which clearly *contains* the word "copy" but whose return value isn't explicitly retained. But Apple does not agree with me, and they refuse to be unambiguous in the docs, even though it's pretty straightforward to be clear.)
Apart from this, I would advice to just avoid any confusion, and rename the method: either name it -valueForAttribute: and autorelease the value, or name it -copyValueForAttribute:.
I wasn't clear in my response. I was addressing the Core Foundation "copy" rule, which is similar to but perhaps theoretically separate from the Cocoa naming convention you cite.
When I cited the rule that values returned by Core Foundation functions containing the word "copy" must be CFReleased when no longer needed, I was referring to the OP's call to the Accessibility API function AXUIElementCopyAttributeValue() in the body of his method. That function contains the word "copy," which tells us that its indirectly returned value has been CFRetained. He is therefore responsible for CFReleasing the object that is returned indirectly in the function's axValue argument, but he does not do that in the body of his method.
It is entirely possible, of course, that the OP did in fact intend to return it from his Cocoa method CFRetained, and that he therefore deliberately included the word "copy" in his method name in order to comply with the Cocoa naming convention. That would be perfectly correct and appropriate, as you point out. But in that case, it would be his responsibility to release the return value from his method when he is done with it. Perhaps he in fact did so (he didn't show us his calling code), but I was guessing that the memory leak he was experiencing came from a failure to do so.
I agree with your suggestion that he should remove the word "copy" from the name of his method if he decides to autorelease the return value. However, I fear that your suggestion that the value returned by his method as it is currently named should be retained because it does contain the word "copy" may confuse him. You have correctly described the naming convention, but in this case the return object of his method is already retained (or, I should say, CFRetained) because it was obtained from an Accessibility API function containing the word "copy." If this was his intent and he properly released the value later, then he doesn't have to change anything and his memory leak is caused by something else entirely.
Actually, in the comment Peter had before the function implementation it says clearly that the value should be returned retained. This was the comment you deleted:
// -valueForAttributeCopy: // Returns an object typed id representing the value for a given attribute of the receiver's AXUIElement or nil if the attribute doesn't exist. // The sender is responsible for releasing the object.
So he was actually doing this right. Later he also explicitly says he's releasing the object.
One thing that's NOT done right though is that [attrNames release] should be *inside* the if block, *or* the attrNames variable should be initialized to nil. Otherwise the release message could be sent to an arbitrary uninitialized pointer, which leads to a crash.
Also, to Peter, have you tried to use Leaks instead of ObjectAlloc? The fact that objects are allocated does not necessarily mean that they're leaked. It could be that some DO proxy object is legitimately holding on to the object for efficiency. Of course it could also be that some DO proxy object is not properly released (as cleaning up NSConnection is pretty messy stuff, and also a bit buggy IMHO).
BTW, the fact that even you (Bill) apparently did not even realize that the value should be expected to be retained perfectly illustrate that I am right in that the rules in the docs are incorrect, or at least not clear enough. Thiswould be an improvement to the 'create' rule that would be much more obvious and completely unambiguous:
"You “create” an object using a method returning an object whose name begins with “alloc”, “new”, “copy”, or “mutableCopy” (for example, alloc, newObject, or mutableCopy)."
I added the "returning an object" to exclude methods like -newDocument: (which, according to the docs, "creates a new document" and also has a name that begins with "new"!)
And having a rule that only refers to the start of the name, you don't have to think about confusing names like these that have "copy" somewhere else in the name.
Christiaan
Turning to your comment on a hypothetical -initWithObjects:copyItems: method in Cocoa, I wonder if you aren't taking the Cocoa naming convention a little too literally. It is true that the method signature contains the word "copy," but it seems clear to me in this case that "copy" is associated with "items" and does not refer to the returned object. The Cocoa methods that include "copyItems" in their signatures, such as -initWithArray:copyItems:, are documented as copying (not retaining) the items in the array. This might be distinguished from the Application Services function PMWorkflowCopyItems(), which is documented to require that the caller release the result.
|