<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/124123/">https://git.reviewboard.kde.org/r/124123/</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 6th, 2015, 3:53 p.m. CEST, <b>Milian Wolff</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="https://git.reviewboard.kde.org/r/124123/diff/5/?file=382833#file382833line69" style="color: black; font-weight: bold; text-decoration: underline;">shell/problemmodelset.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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">69</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">const</span> <span class="n">QList</span><span class="o"><</span><span class="n">ModelData</span><span class="o">>&</span> <span class="n">ProblemModelSet</span><span class="o">::</span><span class="n">models</span><span class="p">()</span> <span class="k">const</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">don't return by const&</p></pre>
</blockquote>
<p>On July 11th, 2015, 8:58 p.m. CEST, <b>Laszlo Kis-Adam</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why not? It returns a reference to the container, <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">NOT</strong> an element in the container.</p></pre>
</blockquote>
<p>On July 11th, 2015, 11:53 p.m. CEST, <b>Aleix Pol Gonzalez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It doesn't buy you anything and it's unsafe.</p></pre>
</blockquote>
<p>On July 12th, 2015, 12:53 a.m. CEST, <b>Laszlo Kis-Adam</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Actually it does save me some resources.
When returning a list, the list needs to be constructed.
When returning a reference to that list, it's just a reference. => Faster and uses less RAM (even if in this case it's very little of a save, imagine it on a larger scale).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As for being unsafe, explain please.
Obviously it can be abused (like storing the reference and dereferencing it after the list no longer exists), but <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">anything</strong> can be abused.</p></pre>
</blockquote>
<p>On July 12th, 2015, 11:24 p.m. CEST, <b>Milian Wolff</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It is not really unsafe, but it couples strongly to a data member, as you cannot return a const& to a temporary. Imagine eventually someone wants to change the implementation, now he could not as he must return a const&.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, your argument with returning a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">const&</code> being faster, that is not true in most cases. Only when you also bind to a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">const&</code> at the callee site will no copy be created, but that you could also achieve when you return a non-<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">const&</code>. NRVO or RVO and/or http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/ give you that. Thus, never return <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">const&</code>.</p></pre>
</blockquote>
<p>On July 13th, 2015, 12:27 a.m. CEST, <b>Laszlo Kis-Adam</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, I obviously meant that you have to use a reference on the callee side too.
Anyhow I'll read up on those optimizations, thanks for the link! :)</p></pre>
</blockquote>
<p>On July 13th, 2015, 12:57 a.m. CEST, <b>Aleix Pol Gonzalez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Laszlo, please don't let this delay the patch, this should have been merged already.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I was not going to.</p></pre>
<br />
<p>- Laszlo</p>
<br />
<p>On July 13th, 2015, 1:52 a.m. CEST, Laszlo Kis-Adam wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Laszlo Kis-Adam.</div>
<p style="color: grey;"><i>Updated July 13, 2015, 1:52 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</div>
<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;">Hi there!
Here's the bottomline of what I've been doing:
Please DO note that this is NOT done yet, I'm only submitting it to get some feedback.
So it should NOT be merged!
(mostly)GUI changes:
====================
- Moved ProblemModel to shell
- Reworked the Problems toolview a little. Now it works like this:
-- ProblemModels are added to ProblemModelSet.
-- ProblemReporterFactory makes instances of ProblemsView.
-- ProblemsView takes the models from ProblemModelSet (also subscribes for updates about them, so if one is added or removed it can add/remove their views) and it provides a tabbed widget where the views for them can be added. It creates instances of ProblemTreeView which show the problems in ProblemModel, and adds them to the tabs. Also the tabs shows the number of problems in the ProblemModels.
-- The toolview will only add actions that are supported by the model (for example: filtering, grouping, reparsing, showing imports. Obviously reparsing doesn't make sense for runtime problem checkers)
See video:
https://www.youtube.com/watch?v=K-wcoXcz-GM
ProblemModel details:
=====================
- Broke up ProblemModel into 2 parts
-- Base ProblemModel that provides the QAbstractItemModel interface for views and can use various ProblemStores to store problems. By default it uses FilteredProblemStore.
-- ProblemReporterModel is basically the old ProblemModel that grabs problems from DUChain, it's a subclass of ProblemModel.
- ProblemStore simply stores problems as a list (well technically it stores them in a tree, but it only has 1 level, so it's a list). There's no filtering, no grouping. It's perfect for ProblemReporterModel since it does filtering itself when grabbing the problems from DUChain.
- FilteredProblemStore DOES filtering, and grouping itself. It stores problems in a tree (ProblemStoreNode subclasses). The tree structure depends on the grouping method, which is implemented with GroupingStrategy subclasses.
- Moved WatchedDocumentSet and it's subclasses from ProblemModel to ProblemStore, as it is really a detail that the model itself doesn't need, however ProblemStore which stores the problems needs it actually.
- Created a new Problem class, DetectedProblem. The intent here was to create a class with a clear interface for problems, which ProblemStore can simply store. Then I wanted to eventually replace the "old" Problem class with it. Then I realized that it's not practical because of the "show imports" feature. Which shows the problems from imported contexts. Unfortunately DUChain is the class that knows those, and it's way too much work to get it out from it. Not to mention it doesn't even make sense, since it's really something that logically belongs there.
Therefore my current plan now is to just bring them under a common interface so that it will not matter at all. The connection to DUchain will be hidden in the implementation.
So the work that's left is creating this common interface and then making these two problem classes use it.
Using this new system is fairly straightforward.
See the test plugin that I've created to test with:
https://bitbucket.org/dfighter1985/kdev-problemtest/src
Especially widget.cpp which instantiates the model, and adds problems, or clears them based on which button is clicked.
Here's a class diagram about all this:
http://i.imgur.com/eGWR3sO.jpg</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;">See the included video.</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>interfaces/CMakeLists.txt <span style="color: grey">(11fa364)</span></li>
<li>interfaces/ilanguagecontroller.h <span style="color: grey">(d76284c)</span></li>
<li>interfaces/iproblem.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>language/backgroundparser/parsejob.cpp <span style="color: grey">(f752207)</span></li>
<li>language/codegen/templateengine_p.h <span style="color: grey">(fcb5a06)</span></li>
<li>language/duchain/navigation/problemnavigationcontext.h <span style="color: grey">(f19aa18)</span></li>
<li>language/duchain/navigation/problemnavigationcontext.cpp <span style="color: grey">(99e2fd0)</span></li>
<li>language/duchain/navigation/problemnavigationcontext.cpp <span style="color: grey">(99e2fd0)</span></li>
<li>language/duchain/problem.h <span style="color: grey">(eb3951a)</span></li>
<li>language/duchain/problem.cpp <span style="color: grey">(8cbf45b)</span></li>
<li>language/duchain/tests/test_duchain.cpp <span style="color: grey">(8d49a24)</span></li>
<li>plugins/problemreporter/CMakeLists.txt <span style="color: grey">(39b3643)</span></li>
<li>plugins/problemreporter/problemhighlighter.h <span style="color: grey">(1df7a79)</span></li>
<li>plugins/problemreporter/problemhighlighter.cpp <span style="color: grey">(b4a6ad2)</span></li>
<li>plugins/problemreporter/problemmodel.h <span style="color: grey">(0d7d773)</span></li>
<li>plugins/problemreporter/problemmodel.cpp <span style="color: grey">(9043a24)</span></li>
<li>plugins/problemreporter/problemreportermodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/problemreporter/problemreportermodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/problemreporter/problemreporterplugin.h <span style="color: grey">(5afd8ad)</span></li>
<li>plugins/problemreporter/problemreporterplugin.cpp <span style="color: grey">(5ca0c8d)</span></li>
<li>plugins/problemreporter/problemsview.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/problemreporter/problemsview.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/problemreporter/problemtreeview.h <span style="color: grey">(b9247d4)</span></li>
<li>plugins/problemreporter/problemtreeview.cpp <span style="color: grey">(1c42034)</span></li>
<li>plugins/problemreporter/watcheddocumentset.h <span style="color: grey">(9dda54d)</span></li>
<li>plugins/problemreporter/watcheddocumentset.cpp <span style="color: grey">(143ee58)</span></li>
<li>shell/CMakeLists.txt <span style="color: grey">(cffa161)</span></li>
<li>shell/filteredproblemstore.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/filteredproblemstore.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/languagecontroller.h <span style="color: grey">(b9a3d06)</span></li>
<li>shell/languagecontroller.cpp <span style="color: grey">(d15a14b)</span></li>
<li>shell/problem.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/problem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/problemmodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/problemmodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/problemmodelset.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/problemmodelset.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/problemstore.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/problemstore.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/problemstorenode.h <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124123/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>