<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/111637/">http://git.reviewboard.kde.org/r/111637/</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 22nd, 2013, 10:14 a.m. UTC, <b>Andreas Pakulat</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/111637/diff/1/?file=172719#file172719line134" style="color: black; font-weight: bold; text-decoration: underline;">language/duchain/types/typeregister.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">class KDEVPLATFORMLANGUAGE_EXPORT TypeSystem {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">134</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Q_ASSERT</span><span class="p">(</span><span class="n">T</span><span class="o">::</span><span class="n">Identity</span> <span class="o"><</span> <span class="mi"><span class="hl">64</span></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">128</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Q_ASSERT</span><span class="p">(</span><span class="n">T</span><span class="o">::</span><span class="n">Identity</span> <span class="o"><</span> <span class="mi"><span class="hl">256</span></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;">What bug does this assert try to prevent? I don't see why the code would limit the number of AbstractType subclasses by an arbitrary number?
If there is a reason then a comment about it would be good to have here IMO.</pre>
</blockquote>
<p>On July 22nd, 2013, 3:06 p.m. UTC, <b>Miquel Sabaté</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;">Hi Andreas. The way I see it, this assert does not try to prevent any bug. The registration of types is kept in a QVector named m_factories. This vector gets resized depending on the Identity that language developers put into their types. So, ideally a developer could put any random (and big) number as an identity, and make this vector huge just for this reason. Therefore, to prevent a potential memory misuse, I guess that David Nolden (from git blame :P), decided that 64 types in the system was enough. However, as more language plugins are being developed, only allowing 64 different types has become insufficient. I think it's a good idea to put a comment before this assert :)</pre>
</blockquote>
<p>On July 22nd, 2013, 6:50 p.m. UTC, <b>Andreas Pakulat</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;">That does not make any sense. A QVector can grow if needed and if you want to ensure that a certain limit is not exceeded because some other part relies on it then it shouldn't be a resizable datatype (i.e. a fixed size array does that job just fine).
As far as I can see from the code, it already supports the resizing so I really don't see the point in having the assert. If performance is a concern then there should be an easy-use-run performance test somewhere that plugin developers can run to see wether their language plugin generates too many types.</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 agree with you that implemeting this functionality in a QVector and then making restrictions on the size of the vector is pointless. This patch was more or less a quick work-around until we get a properly refactored code after all the changes discussed in the KDevelop BoF in the Akademy. Anyways, I'm now more in favor of Milian's patch: https://git.reviewboard.kde.org/r/111643/ . This patch can completely ignore this assert. Therefore, I'll discard this review ;)</pre>
<br />
<p>- Miquel</p>
<br />
<p>On July 22nd, 2013, 3:07 p.m. UTC, Miquel Sabaté 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 Miquel Sabaté.</div>
<p style="color: grey;"><i>Updated July 22, 2013, 3:07 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;">As discussed in the Akademy, it's needed to increase the number of registrable types. Thus, this patch. I've also removed some old code that was commented out and imho was no longer needed.</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;">As an example, I've set the Ruby's ClassType Identity to 250 and everything performed as usual.</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>language/duchain/types/typeregister.h <span style="color: grey">(4843eec)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111637/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>