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
1=== modified file 'src/screen.cpp'
2--- src/screen.cpp 2012-12-02 18:28:53 +0000
3+++ src/screen.cpp 2012-12-04 16:11:25 +0000
4@@ -5077,7 +5077,10 @@
5 * plugins loaded on the command line screen's and then
6 * we need to call updatePlugins () to init the remaining
7 * screens from option changes */
8- assert (CompPlugin::screenInitPlugins (screen));
9+ bool init_succeeded = CompPlugin::screenInitPlugins (screen);
10+ assert (init_succeeded);
11+ if (!init_succeeded)
12+ return false;
13
14 /* The active plugins list might have been changed - load any
15 * new plugins */
16
17=== modified file 'src/window.cpp'
18--- src/window.cpp 2012-11-29 08:14:03 +0000
19+++ src/window.cpp 2012-12-04 16:11:25 +0000
20@@ -6115,7 +6115,10 @@
21 }
22
23 /* TODO: bailout properly when objectInitPlugins fails */
24- assert (CompPlugin::windowInitPlugins (this));
25+ bool init_succeeded = CompPlugin::windowInitPlugins (this);
26+ assert (init_succeeded);
27+ if (!init_succeeded)
28+ return;
29
30 recalcActions ();
31 priv->updateIconGeometry ();

Subscribers

People subscribed via source and target branches