Merge lp:~mc-return/compiz/compiz.fix1030473-part1 into lp:compiz/0.9.8
| Status: | Merged |
|---|---|
| Approved by: | Daniel van Vugt on 2012-08-03 |
| Approved revision: | 3460 |
| Merged at revision: | 3301 |
| Proposed branch: | lp:~mc-return/compiz/compiz.fix1030473-part1 |
| Merge into: | lp:compiz/0.9.8 |
| Diff against target: |
1789 lines (+192/-190) 54 files modified
cmake/src/compiz/compiz_discover_gtest_tests.cpp (+2/-2) compizconfig/libcompizconfig/src/bindings.c (+1/-1) compizconfig/libcompizconfig/src/compiz.cpp (+19/-17) gtk/window-decorator/events.c (+3/-3) kde/window-decorator-kde4/window.cpp (+1/-1) plugins/animation/src/animation.cpp (+8/-8) plugins/animation/src/extensionplugin.cpp (+3/-3) plugins/animationaddon/src/burn.cpp (+1/-1) plugins/annotate/src/annotate.cpp (+1/-2) plugins/ccp/src/ccp.cpp (+1/-1) plugins/clone/src/clone.cpp (+2/-2) plugins/colorfilter/src/parser.cpp (+2/-1) plugins/compiztoolbox/src/compiztoolbox.cpp (+2/-2) plugins/composite/src/screen.cpp (+3/-3) plugins/cubeaddon/src/cubeaddon.cpp (+14/-13) plugins/dbus/src/dbus.cpp (+1/-1) plugins/decor/src/clip-groups/tests/clip-groups/src/test-decor-clip-groups.cpp (+1/-1) plugins/decor/src/decor.cpp (+4/-8) plugins/extrawm/src/extrawm.cpp (+2/-2) plugins/ezoom/src/ezoom.cpp (+2/-3) plugins/firepaint/src/firepaint.cpp (+1/-2) plugins/grid/src/grid.cpp (+4/-4) plugins/group/src/group.cpp (+1/-1) plugins/group/src/init.cpp (+4/-4) plugins/group/src/paint.cpp (+1/-1) plugins/group/src/selection.cpp (+4/-4) plugins/group/src/tab.cpp (+3/-3) plugins/loginout/src/loginout.cpp (+2/-2) plugins/mousepoll/src/mousepoll.cpp (+2/-2) plugins/opengl/src/fragment.cpp (+1/-1) plugins/opengl/src/paint.cpp (+1/-1) plugins/place/src/place.cpp (+1/-1) plugins/place/src/screen-size-change/src/screen-size-change.cpp (+3/-3) plugins/resize/src/resize.cpp (+11/-12) plugins/ring/src/ring.cpp (+6/-6) plugins/scale/src/scale.cpp (+2/-2) plugins/session/src/session.cpp (+6/-8) plugins/shift/src/shift.cpp (+4/-2) plugins/snap/src/snap.cpp (+4/-4) plugins/staticswitcher/src/staticswitcher.cpp (+4/-4) plugins/switcher/src/switcher.cpp (+1/-1) plugins/td/src/3d.cpp (+1/-1) plugins/trailfocus/src/trailfocus.cpp (+2/-2) plugins/wall/src/offset_movement/src/offset-movement.cpp (+1/-1) plugins/wobbly/src/wobbly.cpp (+17/-13) src/event.cpp (+1/-1) src/match.cpp (+2/-2) src/plugin.cpp (+3/-3) src/screen.cpp (+15/-13) src/stackdebugger.cpp (+6/-6) src/timer/src/timeouthandler.cpp (+1/-1) src/timer/src/timer.cpp (+2/-2) src/timer/tests/callbacks/src/test-timer-callbacks.cpp (+1/-1) src/timer/tests/while-calling/src/test-timer-set-times-while-calling.cpp (+1/-1) |
| To merge this branch: | bzr merge lp:~mc-return/compiz/compiz.fix1030473-part1 |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Daniel van Vugt | 2012-07-29 | Approve on 2012-08-03 | |
| MC Return | Resubmit on 2012-07-31 | ||
| Sam Spilsbury | Approve on 2012-07-30 | ||
|
Review via email:
|
|||
Commit Message
Fixed various problems described in bug 1030473.
Optimized performance and style following suggestions reported by cppcheck:
1. Reduced the scope of various variables.
2. Used prefix ++ operators for non-primitive types, because those can be more efficient than post-increment. Post-increment usually keeps a copy of the previous value, adds extra code and is slower.
Description of the Change
Fixes various problems described in bug 1030473.
| MC Return (mc-return) wrote : | # |
| MC Return (mc-return) wrote : | # |
These warnings remain:
[../compiz.
[../compiz.
[../compiz.
%s => %20s
%i => %3i
Sample program that can crash:
#include <stdio.h>
int main()
{
int a;
scanf("%i", &a);
return 0;
}
To make it crash:
perl -e 'print "5"x2100000' | ./a.out
[../compiz.
[../compiz.
%s => %20s
%i => %3i
Sample program that can crash:
#include <stdio.h>
int main()
{
int a;
scanf("%i", &a);
return 0;
}
To make it crash:
perl -e 'print "5"x2100000' | ./a.out
[../compiz.
[../compiz.
[../compiz.
[../compiz.
[../compiz.
%s => %20s
%i => %3i
Sample program that can crash:
#include <stdio.h>
int main()
{
int a;
scanf("%i", &a);
return 0;
}
To make it crash:
perl -e 'print "5"x2100000' | ./a.out
[../compiz.
[../compiz.
%s => %20s
%i => %3i
Sample program that can crash:
#includ...
| MC Return (mc-return) wrote : | # |
Those problems remain:
[../compizconfi
%s => %20s
%i => %3i
Sample program that can crash:
#include <stdio.h>
int main()
{
int a;
scanf("%i", &a);
return 0;
}
To make it crash:
perl -e 'print "5"x2100000' | ./a.out
[../compizconfi
[../compizconfi
%s => %20s
%i => %3i
Sample program that can crash:
#include <stdio.h>
int main()
{
int a;
scanf("%i", &a);
return 0;
}
To make it crash:
perl -e 'print "5"x2100000' | ./a.out
[../compizconfi
[../compizconfi
[../compizconfi
[../compizconfi
[../compizconfi
%s => %20s
%i => %3i
Sample program that can crash:
#include <stdio.h>
int main()
{
int a;
scanf("%i", &a);
return 0;
}
To make it crash:
perl -e 'print "5"x2100000' | ./a.out
[../compizconfi
[../compizconfi
%s => %20s
%i => %3i
Sample program that can crash:
#include <stdio.h>
int main()
{
int a;
scanf("%i", &a);
return 0;
}
To make it crash:
perl -e 'print "5"x2100000' | ./a.out
[../compizconfi
[../compizconfi
[../compizconfi
[../compizconfi
[../compizconfi
[../compizconfi
| Sam Spilsbury (smspillaz) wrote : | # |
7 for (map <string, vector<string> >::iterator it = testCases.begin ();
8 - it != testCases.end (); it++)
9 + it != testCases.end (); ++it)
10 {
11 for (vector <string>::iterator jt = it->second.begin ();
12 - jt != it->second.end (); jt++)
13 + jt != it->second.end (); ++jt)
14 {
15 if (testfilecmake.good ())
16 {
That's fine - you just need to fix the indentation on lines 9 and 13 of the diff (make it line up with the parens)
51 + int j;
52 for (j = 0; j < num; j++)
53 {
Indentation (should be 1 tab)
68 + int j, num = option.
69 for (j = 0; j < num; j++)
70 {
ditto ... there seem to be a lot of places where the indentation isn't quite right. Compiz uses the X11 scheme (which is stupid, but we shouldn't change it)
1 indent: 4 spaces
2 indents: 1 tab (8 wide)
3 indents: 1 tab 4 spaces
4 indents: 2 tabs
etc
You might want to change these:
505 - lastX = -1000000000.0;
506 + float lastX = -1000000000.0, lastZ = 0.0;
507
508 for (i = oldVCount; i < geometry.vCount; i++)
509 {
510 @@ -728,7 +727,7 @@
511 last[0][0] = -1000000000.0;
512 last[1][0] = -1000000000.0;
to
float lastX = std::numeric_limits <float>::min (), rather than their current magic number of -1000000000.0
595 - left = 0;
596 - right = minWidth;
597 - top = 0;
598 - bottom = minHeight;
599 + int left = 0, right = minWidth, top = 0, bottom = minHeight;
Keep these on separate lines, its a bit easier to read
| Daniel van Vugt (vanvugt) wrote : | # |
Lots of indentation needs fixing. Look at the Preview Diff.
I'm not concerned about multiple declarations on one line. It makes sense when the variables are related and/or the line is not very long.
| MC Return (mc-return) wrote : | # |
Daniel, I am sorry about the indentation problems.
I tried to not create any new ones ;), but the diff here shows the code differently than the code editor (QTCreator) that I am currently using - it seems that QTCreator is inserting spaces automatically, when I use Tabulator and I am a bit confused about the strange looking diff here...
Also when reading the code it seemed that inconsistent indentation was already an existing problem...
How about using some specialized tool to fix all of those indentation and stylistic problems automatically ?
I've heard recommendations for the code beautifier "Artistic Style" for example: http://
Also: Are the fixes already done in this branch ok so far, except from the wrong indentation, and do you have any comments on the remaining problems cppcheck found and how to best fix those ?
| MC Return (mc-return) wrote : | # |
The strange thing is that I did not touch the indentation at all, when I changed postfix to prefix operators for example, and it looks correct when I check it in QTCreator, but the diff here says otherwise... :(
- 3424. By MC Return on 2012-07-30
-
Hopefully fixed indentation
- 3425. By MC Return on 2012-07-30
-
Hopefully fixed indentation
- 3426. By MC Return on 2012-07-30
-
Hopefully fixed indentation
- 3427. By MC Return on 2012-07-30
-
Hopefully fixed indentation
- 3428. By MC Return on 2012-07-30
-
Hopefully fixed identation
- 3429. By MC Return on 2012-07-30
-
Hopefully fixed indentation
- 3430. By MC Return on 2012-07-30
-
Hopefully fixed indentation
- 3431. By MC Return on 2012-07-30
-
Hopefully fixed indentation (although grid.cpp does not follow the 4 spaces/tabulator style and uses other method of indentation)
- 3432. By MC Return on 2012-07-30
-
Hopefully fixed indentation
- 3433. By MC Return on 2012-07-30
-
Hopefully fixed indentation
- 3434. By MC Return on 2012-07-30
-
Hopefully fixed indentation
- 3435. By MC Return on 2012-07-30
-
Hopefully fixed indentation
- 3436. By MC Return on 2012-07-30
-
Hopefully fixed indentation
| MC Return (mc-return) wrote : | # |
Sometimes (in the for loops) Tabulator + 5 Spaces were used, I am not sure about those (I used TAB + 4 Spaces)...
The rest of the problems should now be fixed. I hope we can merge this branch ASAP, before starting work on the remaining problems cppcheck detected as those are the more complicated ones and should IMHO not be mixed into one large diff to provide better readability. That is why I named this branch compiz.
- 3437. By MC Return on 2012-07-30
-
Fixed possible performance problem by replacing postfix operator with prefix one for the variable 'it'
- 3438. By MC Return on 2012-07-30
-
Fixed possible performance problem by replacing postfix operator with prefix one for the variables 'it' and 'cur'
- 3439. By MC Return on 2012-07-30
-
Fixed possible performance problem by replacing postfix operator with prefix one for the variable 'it'
- 3440. By MC Return on 2012-07-30
-
Hopefully fixed indentation
- 3441. By MC Return on 2012-07-30
-
Reduced the scope of the variable 'temp'
- 3442. By MC Return on 2012-07-30
-
Reduced the scope of the variable 'pixmap'
- 3443. By MC Return on 2012-07-30
-
Reduced the scope of the variable 'i'
- 3444. By MC Return on 2012-07-30
-
Reduced the scope of the variables 'v', 's' and 'e'
- 3445. By MC Return on 2012-07-30
-
Reduced the scope of the variables 'v', 's' and 'e'
- 3446. By MC Return on 2012-07-30
-
Reduced the scope of the variables 'v', 's' and 'e'
- 3447. By MC Return on 2012-07-30
-
Reduced the scope of the variables 'v', 's' and 'e'
- 3448. By MC Return on 2012-07-30
-
Reduced the scope of the variable 'i'
| Sam Spilsbury (smspillaz) wrote : | # |
Looks good now, the only indentation thing that remains is aligning the next line of the for statements with the content of the statement itself and not the brace, eg
for (....
....)
not
for (....
....)
As for Qt Creator, here are the settings I use:
| MC Return (mc-return) wrote : | # |
Sam, thanks for the comment. I will fix those also, now that I understand how it is supposed to be :)
And thanks a lot for your Qt Creator settings !
| MC Return (mc-return) wrote : | # |
Remaining problems detected:
[../compizconfi
%s => %20s
%i => %3i
Sample program that can crash:
#include <stdio.h>
int main()
{
int a;
scanf("%i", &a);
return 0;
}
To make it crash:
perl -e 'print "5"x2100000' | ./a.out
[../compizconfi
[../compizconfi
%s => %20s
%i => %3i
Sample program that can crash:
#include <stdio.h>
int main()
{
int a;
scanf("%i", &a);
return 0;
}
To make it crash:
perl -e 'print "5"x2100000' | ./a.out
[../compizconfi
[../compizconfi
[../compizconfi
[../compizconfi
[../compizconfi
%s => %20s
%i => %3i
Sample program that can crash:
#include <stdio.h>
int main()
{
int a;
scanf("%i", &a);
return 0;
}
To make it crash:
perl -e 'print "5"x2100000' | ./a.out
[../compizconfi
[../compizconfi
%s => %20s
%i => %3i
Sample program that can crash:
#include <stdio.h>
int main()
{
int a;
scanf("%i", &a);
return 0;
}
To make it crash:
perl -e 'print "5"x2100000' | ./a.out
[../compizconfi
[../compizconfi
[../compizconfi
[../compizconfi
[../compizconfi
[../compizconfi
- 3450. By MC Return on 2012-07-30
-
Fixed indentation
- 3451. By MC Return on 2012-07-30
-
Fixed indentation
- 3452. By MC Return on 2012-07-30
-
Fixed intendation
- 3453. By MC Return on 2012-07-30
-
Fixed intendation
- 3454. By MC Return on 2012-07-30
-
Fixed intendation
- 3455. By MC Return on 2012-07-30
-
Fixed intendation
- 3456. By MC Return on 2012-07-30
-
Fixed intendation
- 3457. By MC Return on 2012-07-30
-
Fixed intendation
- 3458. By MC Return on 2012-07-30
-
Fixed intendation
- 3459. By MC Return on 2012-07-30
-
Fixed intendation
| MC Return (mc-return) wrote : | # |
Okay, should be ready now... :)
| MC Return (mc-return) wrote : | # |
I hope r3460 is okay as well ;) (followed your suggestion in your first comment here).


Still a lot to do :)
[/compizconfig/ compizconfig- python/ compizconfig. c:619]: (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:652]: (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:818]: (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1335] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1343] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1354] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1362] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1370] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1378] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1398] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1431] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1475] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1508] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1532] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:1570] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:2007] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:2057] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:2722] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:3047] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:3136] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:3630] : (style) The second of the two statements can never be executed, and so should be removed. compizconfig- python/ compizconfig. c:3648] : (style) The second of the two stateme...
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/
[/compizconfig/