Merge lp:~bregma/compiz/lp-1085581 into lp:compiz/0.9.9

Proposed by Stephen M. Webb
Status: Merged
Approved by: Didier Roche-Tolomelli
Approved revision: 3501
Merged at revision: 3500
Proposed branch: lp:~bregma/compiz/lp-1085581
Merge into: lp:compiz/0.9.9
Diff against target: 31 lines (+8/-2)
2 files modified
src/screen.cpp (+4/-1)
src/window.cpp (+4/-1)
To merge this branch: bzr merge lp:~bregma/compiz/lp-1085581
Reviewer Review Type Date Requested Status
Didier Roche-Tolomelli Approve
PS Jenkins bot continuous-integration Pending
Review via email: mp+137880@code.launchpad.net

Commit message

Move plugin initialization code out of assert() macro so it still runs even with NDEBUG defined (lp: #1085581).

Description of the change

= Problem description =

Plugins fail to run when compiz is build using the latest cmake.

= The fix =

Turns out the latest cmake adds a macro definition for NDEBUG when building in release mode (where previously it did not do so). That macro removes code inside an assert() macro, and plugin initialization code in compiz was wrapped in such a macro.

This fix moves that code outside of the macro and checks the result of the code in the macro instead.

= Test coverage =

Existing tests were failing: this change should cause those tests to pass once again.

To post a comment you must log in.
lp:~bregma/compiz/lp-1085581 updated
3501. By Stephen M. Webb

Used the new variables so -Wno-unused does not cause trouble with -DNDEBUG.

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

Way saner.

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

WTF!? Who put important code inside asserts?

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

We should probably check the other asserts in the source tree too -
we've been bitten by this change in a few areas now.

On Wed, Dec 5, 2012 at 9:14 AM, Daniel van Vugt
<email address hidden> wrote:
> WTF!? Who put important code inside asserts?
> --
> https://code.launchpad.net/~bregma/compiz/lp-1085581/+merge/137880
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I have searched the whole source tree. These two locations are the only dangerous asserts.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/screen.cpp'
--- src/screen.cpp 2012-12-02 18:28:53 +0000
+++ src/screen.cpp 2012-12-04 16:11:25 +0000
@@ -5077,7 +5077,10 @@
5077 * plugins loaded on the command line screen's and then5077 * plugins loaded on the command line screen's and then
5078 * we need to call updatePlugins () to init the remaining5078 * we need to call updatePlugins () to init the remaining
5079 * screens from option changes */5079 * screens from option changes */
5080 assert (CompPlugin::screenInitPlugins (screen));5080 bool init_succeeded = CompPlugin::screenInitPlugins (screen);
5081 assert (init_succeeded);
5082 if (!init_succeeded)
5083 return false;
50815084
5082 /* The active plugins list might have been changed - load any5085 /* The active plugins list might have been changed - load any
5083 * new plugins */5086 * new plugins */
50845087
=== modified file 'src/window.cpp'
--- src/window.cpp 2012-11-29 08:14:03 +0000
+++ src/window.cpp 2012-12-04 16:11:25 +0000
@@ -6115,7 +6115,10 @@
6115 }6115 }
61166116
6117 /* TODO: bailout properly when objectInitPlugins fails */6117 /* TODO: bailout properly when objectInitPlugins fails */
6118 assert (CompPlugin::windowInitPlugins (this));6118 bool init_succeeded = CompPlugin::windowInitPlugins (this);
6119 assert (init_succeeded);
6120 if (!init_succeeded)
6121 return;
61196122
6120 recalcActions ();6123 recalcActions ();
6121 priv->updateIconGeometry ();6124 priv->updateIconGeometry ();

Subscribers

People subscribed via source and target branches