<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/112039/">http://git.reviewboard.kde.org/r/112039/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 20th, 2013, 12:28 a.m. MSK, <b>Kevin Funk</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/112039/diff/2/?file=183539#file183539line152" style="color: black; font-weight: bold; text-decoration: underline;">debuggers/gdb/registers/registercontroller.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<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">152</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">RegistersGroup</span><span class="o">&</span> <span class="n">IRegisterController</span><span class="o">::</span><span class="n">updateValuesForRegisters</span> <span class="p">(</span> <span class="n">RegistersGroup</span><span class="o">&</span> <span class="n">registers</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;">Something is wrong with this method.
You pass a reference + return the reference in the normal case. So there are basically two ways to get the result out of your method. You have to decide:
a) Pass in a ptr, modify that one, return void.
b) Pass in a const-ref, use that one and return a copy of the result.
This is the style used throughout Qt. Passing around references is (as Milian noted) generally a bad idea.
There are more occurrences of that return-reference-pattern in your diff, please fix those accordingly.</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 don't get it, really. Why is it so bad??
It did it like that because it was very convenient to write something like this:
return convertValuesForGroup (fillValuesForRegisters (getRegistersFromGroupInternally (group) ), format );
Now it takes whole 4 lines of code.
a) It doesn't fit because I don't allocate any memory. And something like this:
getRegistersFromGroupInternally (&group); fillValuesForRegisters(&group)...
IMO looks very odd(because it's not obvious why is it needed to pass in an address not a reference)
b)Nor does this.
Because RegistersGroup is not a simple data type. Creating new copies of this object takes some time/resources, so I don't want to create zillion copies of it without real necessity.
Or is it not an issue here with QT's implicit sharing feature?
So, I pass in a reference and explicitly denote that it's an output parameter.
PS: Still I'd like to know why is it not a good idea to pass around a reference. Because I can't find any reasonable explanation.:(</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 20th, 2013, 12:28 a.m. MSK, <b>Kevin Funk</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/112039/diff/2/?file=183539#file183539line170" style="color: black; font-weight: bold; text-decoration: underline;">debuggers/gdb/registers/registercontroller.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<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">170</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">bool</span> <span class="n">ok</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;">The variable 'ok' is unused, this will result in warning. More occurrences in this patch, please fix those.</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;">Hmm... It's kinda used: ...toUInt(&ok, 16);
I've compiled the whole patch with gcc. There were no warnings at all.
If you use some other compilers and there is actually warnings, please let me know.</pre>
<br />
<p>- Vlas</p>
<br />
<p>On August 19th, 2013, 9:45 p.m. MSK, Vlas Puhov wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Vlas Puhov.</div>
<p style="color: grey;"><i>Updated Aug. 19, 2013, 9:45 p.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;">Supported architectures: x86/x86_64, arm v7(v6 should work too).
Features: x86/x86_64: General registers/flags, segment, FPU registers read/write. XMM - read only.
arm: General registers/flags, VFP single - read/write, other - read only.
Related changes:
DisassembleWidget: removed startAddress and endAddress combo boxes so as to let to resize disassemble view, but the change address functionality is still there through context menu.
FramestackModel: if first frame doesn't contain debug information don't set currentPosition to the first frame with debug information. It allows to keep synchronization between views(gdb, editor, disassemble widget and registers) otherwise all these views'll be in different states.
</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;">Manual only for x86/x86_64 and armv7 architectures </pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=273152">273152</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>debuggers/gdb/registers/registersview.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>debuggers/gdb/registers/registersmanager.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>debuggers/gdb/registers/registersmanager.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>debuggers/gdb/registers/registercontroller_x86.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>debuggers/gdb/registers/registercontroller_x86.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>debuggers/gdb/registers/registercontroller_arm.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>debuggers/gdb/registers/registercontroller_arm.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>debuggers/gdb/registers/registercontroller.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>debuggers/gdb/registers/registercontroller.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>debuggers/gdb/disassemblewidget.cpp <span style="color: grey">(e1d6e8f)</span></li>
<li>debuggers/gdb/disassemblewidget.h <span style="color: grey">(81c25fc)</span></li>
<li>debuggers/gdb/debuggerplugin.cpp <span style="color: grey">(3533cdb)</span></li>
<li>debuggers/gdb/CMakeLists.txt <span style="color: grey">(32ef14f)</span></li>
<li>debuggers/gdb/registers/registersview.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>debuggers/gdb/registers/registersview.ui <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112039/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/08/12/framestackmodel.diff">Related changes in kdevplatform</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>