Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Compilation fix #17209

Merged
merged 3 commits into from
Feb 4, 2017
Merged

Compilation fix #17209

merged 3 commits into from
Feb 4, 2017

Conversation

j-jorge
Copy link
Contributor

@j-jorge j-jorge commented Jan 17, 2017

Please review this pull request. These commits fix various compilation errors on Android, Linux, Mac and iOS.

@j-jorge
Fix various compilation issues.
b81f2bf 
Mostly errors on field initialization order but also missing files
in CMakeLists and missing include directives.
@minggo
Copy link
Contributor

minggo commented Jan 18, 2017

How to reproduce the compiling error?

@j-jorge
Copy link
Contributor Author

j-jorge commented Jan 18, 2017

About commit b81f2bf, the fields initialization order warning messages occur for me when compiling on Linux without -Wno-reorder. This flag is passed to GCC at line 56 of cmake/Modules/SetCompilerOptions.cmake but if you remove it or if you compile without CMake and with -Werror then the build will fail. Here is a sample output from GCC:

./tests/cpp-tests/Classes/UITest/CocoStudioGUITest/UIListViewTest/UIListViewTest.h: In constructor ‘UIListViewTest_Vertical::UIListViewTest_Vertical()’:
./tests/cpp-tests/Classes/UITest/CocoStudioGUITest/UIListViewTest/UIListViewTest.h:59:11: warning: ‘UIListViewTest_Vertical::_updateTimer’ will be initialized after [-Wreorder]
     float _updateTimer;
           ^~~~~~~~~~~~
./tests/cpp-tests/Classes/UITest/CocoStudioGUITest/UIListViewTest/UIListViewTest.h:58:11: warning:   ‘float UIListViewTest_Vertical::_updateInterval’ [-Wreorder]
     float _updateInterval;
           ^~~~~~~~~~~~~~~
./tests/cpp-tests/Classes/UITest/CocoStudioGUITest/UIListViewTest/UIListViewTest.cpp:22:1: warning:   when initialized here [-Wreorder]
 UIListViewTest_Vertical::UIListViewTest_Vertical()
 ^~~~~~~~~~~~~~~~~~~~~~~

The missing files in CMakeLists.txt and various missing imports result in errors at compile and link time on OSX by typing cmake . && make in a freshly cloned repository.

About commit f54660a, GCC 6 introduces the -Wmisleading-indentation warning for misleading indentation, triggerred when an instruction seems to be part of a block while it is semantically outside the block, and the -Wnonnull-compare trigerred when a pointer that cannot be null (typically this) is compared with nullptr. Here is the error messages from CCArray.cpp:

./cocos/deprecated/CCArray.cpp: In member function ‘virtual cocos2d::__Array* cocos2d::__Array::clone() const’:
./cocos/deprecated/CCArray.h:172:5: error: nonnull argument ‘this’ compared to NULL [-Werror=nonnull-compare]
     if ((__array__) && (__array__)->data->num > 0)                                                                     \
     ^
../../../../../../extensions/cocos/cocos/deprecated/CCArray.cpp:739:5: note: in expansion of macro ‘CCARRAY_FOREACH’
     CCARRAY_FOREACH(this, obj)
     ^~~~~~~~~~~~~~~

About commit f4313ca, I cannot reproduce the error, for an unknown reason.

About commit 994f728, some .jar files are required for the Android builds (for example the content of cocos/platform/android/java/libs/) but they are ignored by .gitignore. Thus these files cannot be updated and a git archive export excludes them, making the export useless.

minggo
minggo reviewed Jan 19, 2017
View reviewed changes
cocos/3d/CCMesh.cpp
, _glProgramState(nullptr)
, _blend(BlendFunc::ALPHA_NON_PREMULTIPLIED)
, _visibleChanged(nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_visibleChanged is missed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_visibleChanged is an std::function and initializing an std::function with nullptr is equivalent to using its default constructor. Thus there is no need to keep this line.

minggo
minggo reviewed Jan 19, 2017
View reviewed changes
cocos/deprecated/CCArray.cpp
@@ -350,7 +350,8 @@ __Array* __Array::clone() const
Ref* obj = nullptr;
Ref* tmpObj = nullptr;
Clonable* clonable = nullptr;
CCARRAY_FOREACH(this, obj)
const __Array* self( this );
CCARRAY_FOREACH(self, obj)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't quite understand it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expansion of CCARRAY_FOREACH will begin with if (this) but since GCC 6 this is expected not to be null. Here is a quote from GCC's release notes:

Value range propagation now assumes that the this pointer of C++ member functions is non-null.

Thus this test triggers the -Wnonnull-compare warning of GCC. Using another pointer (self) in the test prevents the warning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to know, thanks

minggo
minggo reviewed Jan 19, 2017
View reviewed changes
cocos/deprecated/CCDeprecated.h
@@ -986,9 +986,6 @@ CC_DEPRECATED_ATTRIBUTE const TransitionScene::Orientation kCCTransitionOrientat

CC_DEPRECATED_ATTRIBUTE typedef TransitionScene::Orientation tOrientation;

CC_DEPRECATED_ATTRIBUTE const int kCCPrioritySystem = Scheduler::PRIORITY_SYSTEM;
CC_DEPRECATED_ATTRIBUTE const int kCCPriorityNonSystemMin = Scheduler::PRIORITY_NON_SYSTEM_MIN;

Copy link
Contributor

@minggo minggo Jan 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove these codes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines can be kept, I will revert the commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert it, then i will merge this pr, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit removed from the branch :)

@minggo minggo added this to the 3.15 milestone Feb 4, 2017
@minggo minggo merged commit c616d6d into cocos2d:v3 Feb 4, 2017
stevetranby added a commit to stevetranby/cocos2d-x that referenced this pull request Feb 13, 2017
@stevetranby
Merge branch 'v3' into scgamex-futurePRs
2178390 
* v3:
  fix the bug that ect1 texture lost on android (cocos2d#17278)
  Included the directories such that the extension classes work out of the box in Linux (cocos2d#17259)
  Prevent removal of unknown touch in CCScrollView::onTouchCancelled. (cocos2d#17208)
  Fix implicit conversion warnings (cocos2d#17254)
  Avoid variable shadowing (cocos2d#17264)
  Fix typo in function name (cocos2d#17253)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#17276)
  Fix typo in AudioState (cocos2d#17258)
  Fixed variable order in initializer lists (cocos2d#17274)
  Compilation fix (cocos2d#17209)
  Allow to unbind asynchronous texture loading callback with a custom key. (cocos2d#17206)

# Conflicts:
#	cocos/2d/CCParticleBatchNode.cpp
#	cocos/3d/CCMeshSkin.cpp
#	cocos/3d/CCSkeleton3D.cpp
#	cocos/network/SocketIO.cpp
#	cocos/ui/UIEditBox/Mac/CCUIEditBoxMac.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minggo minggo minggo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.15
Development

Successfully merging this pull request may close these issues.
2 participants
@j-jorge @minggo