-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
issue #1564: Return a new object instead of 'const reference' for som…
…e get methods.
- Loading branch information
James Chen
committed
Nov 20, 2012
1 parent
4d39ecf
commit b8b2af0
Showing
43 changed files
with
109 additions
and
157 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
b8b2af0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you replace returning of const reference to returning of copy?
b8b2af0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it is easy to return a reference to a temple variable, which we met many times. So it is more safe to return a copy. I know it will take more memory, but it is not called frequently.
So i think it is not so bad.
May be i am wrong.
b8b2af0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @moadib,
It's because this interface is virtual, and it returns a reference of member variable, but it's subclass may return a temporary variable, it will cause crashes. And it's a dangerous operation if returning reference when the function is a virtual function.
b8b2af0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anytime when you try return reference to local or temporary object c++ compiler gives you C4172 warning, isn't it?
Forewarned is forearmed. We talk about performance-critical applications so such things important. Of course I can continue to support our internal version of cocos2d-x without this stuff, but I want to cocos2dx was better.
Also as i can see CCPoint still inherited from CCObject. If this ugly inheritance needed only for CatMull-splines maybe you must surround it with I_WANT_USE_CATMULL which must be named I_WANT_USE_CATMULL_AND_WARNED_ABOUT_PERFORMANCE__LOSS_AND_MEMORY_CONSUMPTION in fact?
Same i can suggest for script-support-specific variables in CCObject.
b8b2af0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moadib
I think you are right. We should change them back.
Thank you.
About the variable in CCObject. It is better to use macro to control it. Because cocos2d-x windows project is shared for all samples. So it is hard to use macro.
Any idea?
b8b2af0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created issues for them.
http://www.cocos2d-x.org/issues/1649
http://www.cocos2d-x.org/issues/1648
b8b2af0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe then
and
I_DONT_NEED_CATMULL_SPLINES
?Then user can set this defines for his project and all tests and samples will work as usual.
@minggo Thanks! I have a dream that one day I can use cocos2d-x unchanged. And that day is getting closer and closer.
b8b2af0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On windows, the libcocos2dx is shared for TestCpp, TestJavascript, HelloLua. And the macro defined in TestJavascript will not affect the configuration of libcocos2dx.
It could be done by adding libcocos2dx-lua, libcocos2dx-js, but there will be three engine probjects.
b8b2af0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is something to think about...
b8b2af0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @moadib, let me know if you find the way. Thanks.
b8b2af0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @moadib
I have reverted this commit here(#1838).
Thanks.