Review Request 111637: Increased the number of registrable types to 256.

[Miquel Sabaté]

> On July 22, 2013, 10:14 a.m., Andreas Pakulat wrote:
> > language/duchain/types/typeregister.h, line 134
> > <http://git.reviewboard.kde.org/r/111637/diff/1/?file=172719#file172719line134>
> >
> >     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.
> 
> Miquel Sabaté wrote:
>     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 :)
> 
> Andreas Pakulat wrote:
>     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.

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 ;)


- Miquel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111637/#review36281
-----------------------------------------------------------


On July 22, 2013, 3:07 p.m., Miquel Sabaté wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111637/
> -----------------------------------------------------------
> 
> (Updated July 22, 2013, 3:07 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   language/duchain/types/typeregister.h 4843eec 
> 
> Diff: http://git.reviewboard.kde.org/r/111637/diff/
> 
> 
> Testing
> -------
> 
> As an example, I've set the Ruby's ClassType Identity to 250 and everything performed as usual.
> 
> 
> Thanks,
> 
> Miquel Sabaté
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20130722/c02526b1/attachment.html>