Proposal for v3.0: "protect" the constructors

Hi,

Introduction:

cocos2d uses 2-phase initialization for the objects:

  • new Object
  • init()

Which allow us to return an error if something goes wrong, instead of throwing an exception.

And we have the convenience static method that calls those 2 functions:

static Object* create() {
  auto ret = new Object;
  if( ret && ret->init() )
     ret->autorelease();
  return ret;
}

And on init() we usually initialize some non POD ivars:
eg:

bool Object::init() {
  // do something
  return true;
}

Why do I want to “protect” the constructor ?

Basically because doing this is wrong:

auto array = new Array(); // without the init

or worse:

class MyClass {
  Array _myArray;  // not a pointer
};

And this is also wrong:

// OK
auto array = Array::create();
array->retain();

...

// WRONG... it should be CC_SAFE_RELEASE(array);
CC_SAFE_DELETE(array);

So, our API is error prone. We shouldn’t expose an API that is very easy to create errors.
It is so error prone, that I saw those bugs in our own code (in the cocos2d-x code).

So, my proposal is to “protect” the constructors.

eg:

class Array {
public:
   Array* create();

protect:
   Array();

}

So if you want to create an object, the only way to create it is by using the createXXX methods.

Pros

  • Will detect CC_SAFE_DELETE bugs (it won’t compile)
  • Will detect non-initialized objects (it won’t compile)
  • Will detect potential memory leaks (new without delete) (it won’t compile)
  • Has a more uniform API: easier to use, easier to remember
  • Additionally (as a side effect), it will be easier to migrate to an smart_ptr API in case we need it

Cons

  • All the created objects will be autoreleased (that means that they will be copied to the autorelease pool)

Thoughts?

Sounds like factory method design pattern :slight_smile:

I agree with the design, and corresponding initxxx functions should be protected too.

Just to be clear, you mean protected keyword, right? I googled protect keyword on C*+ and nothing showed up. I started to think there was a new keyword protect in C*+11. o_O

And yes, I agree. :slight_smile:

@Jan Michael Chuaquico
Yep, it is protected.

winipcfg winipcfg wrote:

Sounds like factory method design pattern :slight_smile:

Yes it is.
Currently most of cocos2d objects already have the createXXX functions, but we are not enforcing them.
So my proposal basically is to enforce it: the only way to create such objects should be via the createXXX methods.

Looks good to me, and if we use smart pointers or the hybrid approach the autorelease pool issue will be gone.

As Minggo said, the init methods should be protected too as they are designed to be called only once (otherwise a lot of memory leaks will appear)
So this code should not be possible:

bool MyObject::init() {
  // [...]
  mySprite->initWithSpriteFrame(spriteFrame);
  // [...]
}

void MyObject::foo() {
  // reinit the same sprite instance, will result in memory leaks
  mySprite->initWithFile(someFileName);
}

Ricardo Quesada wrote:

Currently most of cocos2d objects already have the createXXX functions, but we are not enforcing them.
So my proposal basically is to enforce it: the only way to create such objects should be via the createXXX methods.

I agree to make all initXXX method to protected.
But there is a problem. That’s JS-Binding and Lua-Binding need initXXX to be public method.
Consider currently binding codes:

JSBool js_cocos2dx_Sprite_initWithFile(JSContext *cx, uint32_t argc, jsval *vp)   // Notice: This is a C function, we could not make it as the friend function of Sprite. If so, lots of method need to be friend methods.
{
...
    do {
        if (argc == 1) {
            const char* arg0;
            std::string arg0_tmp; ok &= jsval_to_std_string(cx, argv[0], &arg0_tmp); arg0 = arg0_tmp.c_str();
            if (!ok) { ok = JS_TRUE; break; }
            bool ret = cobj->initWithFile(arg0);
            jsval jsret;
            jsret = BOOLEAN_TO_JSVAL(ret);
            JS_SET_RVAL(cx, vp, jsret);
            return JS_TRUE;
        }
    } while(0);
...
}

This way of invocation needs the method initWithFile to be public. Otherwise, there will be lots of complication errors.

How about don’t expose initXXX method to JS or Lua.

That is impracticable. Since in JS, developer may inherit the classes that exposed to JS.

For instance,

var MySprite = cc.Sprite.extend({
    ctor: function() {
        this._super();
        this.initWithFile(filename);   // initWithFile is the parent's method.
    }
});

Therefore, initWithXXX need to be public methods in Bindings.
If this problem could be fixed. I vote for making the initXXX method to be protected. :slight_smile:

Agreed to the change. :slight_smile:
It will be quite useful to work with entity/component system design.

Sounds like a sensible change to me.

Ben

James Chen wrote:

h2. How about don’t expose initXXX method to JS or Lua.
>
That is impracticable. Since in JS, developer may inherit the classes that exposed to JS.

If you forget about the current model and how the current inheritance model work, is it possible to subclass native classes from JSB by just using createXXX ?

It might work without current model. But I’m not a JS expert, I will ask WuHao andDavid for help. Thanks.

Ricardo Quesada wrote:

James Chen wrote:
>
> h2. How about don’t expose initXXX method to JS or Lua.
>
> That is impracticable. Since in JS, developer may inherit the classes that exposed to JS.
>
If you forget about the current model and how the current inheritance model work, is it possible to subclass native classes from JSB by just using createXXX ?

As i know, JS can not protect a function from being invoked. And JSB should keep the same interface as html5. So it is a problem.

Ricardo Quesada wrote:

James Chen wrote:
>
> h2. How about don’t expose initXXX method to JS or Lua.
>
> That is impracticable. Since in JS, developer may inherit the classes that exposed to JS.
>
If you forget about the current model and how the current inheritance model work, is it possible to subclass native classes from JSB by just using createXXX ?

I don’t understand what does the “subclass native classes from JSB by just using createXXX” mean.

Could you please give some pseudocode example?

We dont’t use the method: initWithFile and some initXX in our game, which is developed by jsbinding,
because it should be crush the game some times. So if dont’t expose initXX to js may be OK for developer.

Ricardo Quesada wrote:

James Chen wrote:
>
> h2. How about don’t expose initXXX method to JS or Lua.
>
> That is impracticable. Since in JS, developer may inherit the classes that exposed to JS.
>
If you forget about the current model and how the current inheritance model work, is it possible to subclass native classes from JSB by just using createXXX ?

Dingping Lv wrote:

I don’t understand what does the “subclass native classes from JSB by just using createXXX” mean.
>
Could you please give some pseudocode example?

Hi,

What I was saying is:

  • Assume that the only way to create objects in cocos2d-x is by using createXXX
  • Assume that initXXX is protected in cocos2d-x

With that in mind, if you are asked to create JS bindings from scratch for cocos2d-x, how would you do it ? is it possible to do it ? (forget about the current code).
thanks.

Hi Riq,

Got it.
It will be very expensive to hide class’s constructor, because JS can not protect a function from being invoked. Some details for reference: http://stackoverflow.com/questions/16246256/private-constructor-in-javascript-with-static-members
The most ~~html5 developers create objects with createXXX, but there still several developers use it like this: var aSprite = new cc.Sprite; aSprite.initWithXXX;
Because jsb doesn’t support subclass inherit from native class to overload its protected function, so we cannot do this in JSB like the following code:

var MySprite = cc.Sprite.extend({ ctor: function() { this._super(); this.initWithFile(filename); // initWithFile is the parent's method. } });
I think we should use the conventional way to do it, and give advice to all developers to create objects with CreateXXX. We should modify all initXXX function of~~html5 to _initXXX to let users to know that it’s protected.
If users still use “var aSprite = new cc.Sprite(); aSprite._initWithXXX();” to create an object, it will be broken on jsb. It is not our responsibility, what we can do is let them to know this.

Thanks
David

Seems like everybody is forgetting about value types here!
One should be able to create temporal objects like this:

void dummyMethod()
{
    CCString str;
    str.initWithFormat("Say something: %s", "Hello World!");
}
// No memory leaks, no autorelease, no dynamic memory (on the client side), no problems!

This is obviously usefull for some of the cocoa-ported classes (CCPoint, CCString, CCRect, CCSize…), but i’ve sometimes used this for CCSpriteFrame as well.
This is the strength of C++, if we block this we may be programming in Java as we lose the ability to put objects into the stack.

@Xavier Arias
Yep. If we protect constructor and init functions, then it is impossible to create an object in stack. Is it any problem for that?

@Minggo Zhang
Of course that’s a problem! At least with some of the most basic cc types (CCPoint, CCSize, CCRect, CCString…) it’s imperative to keep it.

The way I see it, you should only protect CCNode’s and CCNode’s subclasses constructors.
The rest of classes can be used at the stack like temporary variables.

Even collections like CCArray could be useful in some cases to use inside methods as local variables.
You must note that all std classes support this. Value types is one of the core features of C++.