[prev in list] [next in list] [prev in thread] [next in thread]
List: kwin
Subject: Re: Review Request: 1 leak,
From: "Jaime Torres Amate" <jtamate () gmail ! com>
Date: 2011-07-26 6:57:55
Message-ID: 20110726065755.29818.39623 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On July 25, 2011, 8:48 p.m., Martin Gräßlin wrote:
> > I will give the library deletion a try, that it does not break anything (luckily \
> > I have currently one that doesn't work).
> > Apart from that the patch seems mostly fine, although I still doubt that \
> > compilers are stupid and generate slower code for i++ than ++i ;-).
It should not break anything, it just deletes a variable that is allocated and will \
never be used again.
> On July 25, 2011, 8:48 p.m., Martin Gräßlin wrote:
> > kwin/clients/oxygen/oxygenexceptionlist.cpp, line 68
> > <http://git.reviewboard.kde.org/r/102057/diff/2/?file=29512#file29512line68>
> >
> > why only prefix iter and not index?
http://www.tantalon.com/pete/cppopt/asyougo.htm
Because index is a trivial-type, where the compiler does generates the same code when \
the optimizations are enabled.
> On July 25, 2011, 8:48 p.m., Martin Gräßlin wrote:
> > kwin/kcmkwin/kwinrules/ruleswidget.cpp, lines 448-449
> > <http://git.reviewboard.kde.org/r/102057/diff/2/?file=29518#file29518line448>
> >
> > I don't think this check makes much sense here. Yes if rules is still a pointer \
> > to tmp, it will point to invalid memory as soon as the method returns.
> > On the other hand if we set rules to NULL it will crash as soon as the calling \
> > function tries to use rules without checking (which I doubt it does).
> > The code has worked for years so I think that rules is never null when entering \
> > the method (the assingement is clearly wrong). So a proper fix in my opinion is \
> > to add an assert as first line of the method to check whether rules is NULL.
I'll change it to an assert, you are sure that path will never happen as it has not \
happened until today.
> On July 25, 2011, 8:48 p.m., Martin Gräßlin wrote:
> > kwin/tools/decobenchmark/preview.cpp, line 123
> > <http://git.reviewboard.kde.org/r/102057/diff/2/?file=29520#file29520line123>
> >
> > out of interest: how did you find this one? It is in more or less dead code which \
> > I personally had never compiled.
The same way as the other things.... cppcheck.
- Jaime Torres
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102057/#review5107
-----------------------------------------------------------
On July 23, 2011, 10:12 a.m., Jaime Torres Amate wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102057/
> -----------------------------------------------------------
>
> (Updated July 23, 2011, 10:12 a.m.)
>
>
> Review request for kwin.
>
>
> Summary
> -------
>
> Some minor performance .. prefix ++ for non atomic types.
> Remove the leak for library.
> Reduce the scope for two variables.
> Avoid the modification of a parameter pointer pointing to the local stack when it \
> was NULL. Avoid the use of deco before it is sure is not NULL.
>
>
> Diffs
> -----
>
> kwin/clientgroup.cpp f8604b6
> kwin/clients/oxygen/oxygenexceptionlist.cpp 652dda0
> kwin/effects.cpp 143b033
> kwin/effects/blur/blurshader.cpp e7e5bf3
> kwin/effects/coverswitch/coverswitch.cpp 6bc98b3
> kwin/effects/presentwindows/presentwindows.cpp a0e8306
> kwin/effects/slidingpopups/slidingpopups.cpp 34bdac4
> kwin/kcmkwin/kwinrules/ruleswidget.cpp c3a44df
> kwin/libkwineffects/kwineffects.cpp e0c5538
> kwin/tools/decobenchmark/preview.cpp 36cfcd1
> kwin/useractions.cpp 387e499
> kwin/workspace.cpp 1a48194
>
> Diff: http://git.reviewboard.kde.org/r/102057/diff
>
>
> Testing
> -------
>
> Working with the patch applied.
>
>
> Thanks,
>
> Jaime Torres
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/102057/">http://git.reviewboard.kde.org/r/102057/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On July 25th, 2011, 8:48 p.m., <b>Martin \
Gräßlin</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">I will give the library deletion a try, that it does not break anything \
(luckily I have currently one that doesn't work).
Apart from that the patch seems mostly fine, although I still doubt that compilers \
are stupid and generate slower code for i++ than ++i ;-).</pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It should not break \
anything, it just deletes a variable that is allocated and will never be used \
again.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On July 25th, 2011, 8:48 p.m., <b>Martin \
Gräßlin</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://git.reviewboard.kde.org/r/102057/diff/2/?file=29512#file29512line68" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kwin/clients/oxygen/oxygenexceptionlist.cpp</a> <span \
style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
">namespace Oxygen</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">68</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">for</span><span class="p">(</span> <span \
class="n">ExceptionList</span><span class="o">::</span><span \
class="n">const_iterator</span> <span class="n">iter</span> <span class="o">=</span> \
<span class="n">constBegin</span><span class="p">();</span> <span \
class="n">iter</span> <span class="o">!=</span> <span class="n">constEnd</span><span \
class="p">();</span> <span class="n">iter</span><span class="o"><span \
class="hl">++</span></span><span class="p">,</span> <span class="n">index</span><span \
class="o">++</span> <span class="p">)</span></pre></td> <th bgcolor="#e9eaa8" \
style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">68</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">for</span><span class="p">(</span> <span \
class="n">ExceptionList</span><span class="o">::</span><span \
class="n">const_iterator</span> <span class="n">iter</span> <span class="o">=</span> \
<span class="n">constBegin</span><span class="p">();</span> <span \
class="n">iter</span> <span class="o">!=</span> <span class="n">constEnd</span><span \
class="p">();</span> <span class="o"><span class="hl">++</span></span><span \
class="n">iter</span><span class="p">,</span> <span class="n">index</span><span \
class="o">++</span> <span class="p">)</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">why only prefix iter and \
not index?</pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">http://www.tantalon.com/pete/cppopt/asyougo.htm
Because index is a trivial-type, where the compiler does generates the same code when \
the optimizations are enabled.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On July 25th, 2011, 8:48 p.m., <b>Martin \
Gräßlin</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://git.reviewboard.kde.org/r/102057/diff/2/?file=29518#file29518line448" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kwin/kcmkwin/kwinrules/ruleswidget.cpp</a> <span style="font-weight: \
normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void \
RulesWidget::setRules(Rules* rules)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">448</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">if</span> <span class="p">(</span><span class="n">rules</span> <span \
class="o">==</span> <span class="o">&</span><span class="n">tmp</span><span \
class="p">)</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">449</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">rules</span> <span class="o">=</span> <span class="nb">NULL</span><span \
class="p">;</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't think this \
check makes much sense here. Yes if rules is still a pointer to tmp, it will point to \
invalid memory as soon as the method returns.
On the other hand if we set rules to NULL it will crash as soon as the calling \
function tries to use rules without checking (which I doubt it does).
The code has worked for years so I think that rules is never null when entering the \
method (the assingement is clearly wrong). So a proper fix in my opinion is to add an \
assert as first line of the method to check whether rules is NULL.</pre> \
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'll \
change it to an assert, you are sure that path will never happen as it has not \
happened until today.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On July 25th, 2011, 8:48 p.m., <b>Martin \
Gräßlin</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://git.reviewboard.kde.org/r/102057/diff/2/?file=29520#file29520line123" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kwin/tools/decobenchmark/preview.cpp</a> <span style="font-weight: \
normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void \
KDecorationPreview::performRecreationTest(int n)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">122</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">deco</span><span class="o">-></span><span class="n">init</span><span \
class="p">();</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">out of interest: how did \
you find this one? It is in more or less dead code which I personally had never \
compiled.</pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The same \
way as the other things.... cppcheck.</pre> <br />
<p>- Jaime Torres</p>
<br />
<p>On July 23rd, 2011, 10:12 a.m., Jaime Torres Amate wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for kwin.</div>
<div>By Jaime Torres Amate.</div>
<p style="color: grey;"><i>Updated July 23, 2011, 10:12 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Some minor performance .. prefix ++ for non atomic types. Remove the \
leak for library. Reduce the scope for two variables.
Avoid the modification of a parameter pointer pointing to the local stack when it was \
NULL. Avoid the use of deco before it is sure is not NULL.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Working with the patch applied.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kwin/clientgroup.cpp <span style="color: grey">(f8604b6)</span></li>
<li>kwin/clients/oxygen/oxygenexceptionlist.cpp <span style="color: \
grey">(652dda0)</span></li>
<li>kwin/effects.cpp <span style="color: grey">(143b033)</span></li>
<li>kwin/effects/blur/blurshader.cpp <span style="color: grey">(e7e5bf3)</span></li>
<li>kwin/effects/coverswitch/coverswitch.cpp <span style="color: \
grey">(6bc98b3)</span></li>
<li>kwin/effects/presentwindows/presentwindows.cpp <span style="color: \
grey">(a0e8306)</span></li>
<li>kwin/effects/slidingpopups/slidingpopups.cpp <span style="color: \
grey">(34bdac4)</span></li>
<li>kwin/kcmkwin/kwinrules/ruleswidget.cpp <span style="color: \
grey">(c3a44df)</span></li>
<li>kwin/libkwineffects/kwineffects.cpp <span style="color: \
grey">(e0c5538)</span></li>
<li>kwin/tools/decobenchmark/preview.cpp <span style="color: \
grey">(36cfcd1)</span></li>
<li>kwin/useractions.cpp <span style="color: grey">(387e499)</span></li>
<li>kwin/workspace.cpp <span style="color: grey">(1a48194)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102057/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
kwin mailing list
kwin@kde.org
https://mail.kde.org/mailman/listinfo/kwin
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic