CCArray::copyWithZone issue

cocos2d-x C++
#1

Hello.
I was working with CCArray and got to a double release of an object.
While scanning over the code I found out the CCArray copies itself like so:

`CCObject* CCArray::copyWithZone(CCZone* pZone)
{
CCAssert(pZone == NULL, “CCArray should not be inherited.”);
CCArray* pArray = new CCArray();
pArray->initWithCapacity(this->data->num > 0 ? this->data->num : 1);

CCObject* pObj = NULL;
CCObject* pTmpObj = NULL;
CCARRAY_FOREACH(this, pObj)
{
    pTmpObj = pObj->copy();
    pArray->addObject(pTmpObj);
    +*pTmpObj->release();*+
}
return pArray;

}`

I underlined the line which is a bug.
You add the object to the array which retains it and then releases it
while, I guess, you assume that the “copy()” of the object retains it (which it may very well not, as in my case).
At first I thought that I missed some guidelines which says you have to retain on copy,
but after I could not find any I noticed that even the very same copy here does not use retain.

Some extra non-crucial, but related, ideas of mine:

  1. pTmpObj is a terrible name for this object because it is really really not a temp, it is being added and kept in the array
  2. If you for some unknown reason decide that retain is a must on copy then please just add a retain at CCObject::copy()

Thanks and have a nice day.

P.S:
Working for a little while with cocos2d-x now and I am, so far, very satisfied.
Please keep up the good work, and I hope this contributed by some means.

#2

>At first I thought that I missed some guidelines which says you have to retain on copy,
>but after I could not find any I noticed that even the very same copy here does not use retain.
It’s common approach in Objective-C, and copied in cocos2d-x. Copying is chinese national sport :wink:

However, it’s very useful feature. And yes, copy() method always should retain.

#3

Sergey Shambir wrote:

>At first I thought that I missed some guidelines which says you have to retain on copy,
>but after I could not find any I noticed that even the very same copy here does not use retain.
It’s common approach in Objective-C, and copied in cocos2d-x. Copying is chinese national sport :wink:
>
However, it’s very useful feature. And yes, copy() method always should retain.

Why do you believe that the copy() should always retain?
In cocos2d you usually have to retain everything yourself as long as you are the pointer keeper,
it is not usual for you to get a pointer wihtout new/malloc and not be needing to retain it…

Ho, and did you mean that every copyWithZone should be implemented with a retain or that CCObject::copy() itself should have it?

#4

>Why do you believe that the copy() should always retain?
Because it’s requirement in Objective-C: http://developer.apple.com/library/mac/#documentation/cocoa/reference/foundation/Protocols/NSCopying_Protocol/Reference/Reference.html#//apple_ref/occ/intfm/NSCopying/copyWithZone:

>Ho, and did you mean that every copyWithZone should be implemented with a retain or that CCObject::copy() itself should have it?
In cocos2d-x, copy() should return retained object (or just created with new Class() and not autoreleased).

#5

Sergey Shambir wrote:

>Why do you believe that the copy() should always retain?
Because it’s requirement in Objective-C: http://developer.apple.com/library/mac/#documentation/cocoa/reference/foundation/Protocols/NSCopying_Protocol/Reference/Reference.html#//apple_ref/occ/intfm/NSCopying/copyWithZone:
>
>Ho, and did you mean that every copyWithZone should be implemented with a retain or that CCObject::copy() itself should have it?
In cocos2d-x, copy() should return retained object (or just created with new Class() and not autoreleased).

I think that retaining is not the best idea.
About a new, I do think it is a good solution, but remember that copy uses the copyWithZone which is implemented by inheriting classes and does not create the object on its own.
I really do think that the best practice is to assume no retain was made and let the “user” decide whether to reatin/autorelease/only new/etc…

#6

>About a new, I do think it is a good solution, but remember that copy uses the copyWithZone which is implemented by inheriting classes and does not create the object on its own.
No, it can create object if it still not created. See implementation of CCSpeed::copyWithZone(CCZone *pZone).

>I really do think that the best practice is to assume no retain was made and let the “user” decide whether to reatin/autorelease/only new/etc…

template 
T *CCAutoCopy(T *object)
{
    return static_cast(object->copy()->autorelease());
}