Review Request 115446: Further kdev-clang code completion improvements.

[Milian Wolff]

> On Feb. 3, 2014, 1:15 p.m., Milian Wolff wrote:
> > codecompletion/context.cpp, line 297
> > <https://git.reviewboard.kde.org/r/115446/diff/1/?file=241520#file241520line297>
> >
> >     while this does help showing the overloaded items, it does so in a very costly way (repeatedly looking up the qid declarations). So rather, I suggest keeping the existing qid-based QSet but adding items for all overloaded items at once.
> 
> Sergey Kalinichev wrote:
>     Are you sure this is an issue, especially when there is usually no more than 3-5 overloaded methods as compared to ~100 usual, so the performance penalty IMO is not so big +/-10 ms.
>       Otherwise adding all overloaded items at once will significantly complicate the code (as all items will have the same display value and we somehow have to retrieve the correct ones...).
>     So I suggest we leave it that way and if time shows that it's really a hot spot we could come up with a better solution then.
>

Ah right, I didn't think about the display values. So yes, leave it as-is for now.


> On Feb. 3, 2014, 1:15 p.m., Milian Wolff wrote:
> > codecompletion/context.cpp, line 305
> > <https://git.reviewboard.kde.org/r/115446/diff/1/?file=241520#file241520line305>
> >
> >     this should instead use ClassDeclaration::isPublicBaseClass and its argument to find the depth. Or maybe even found->context()->depth() ?
> 
> Sergey Kalinichev wrote:
>     Well, nothing of that solves the problem.
>     Here is what I wanted to achieve by doing this:
>     snippet:  int i; void f(int j); f()
>     Pressing crt+space inside of f() will suggest a lot of global items and "i" variable somewhere in the middle of it, to prevent this I wanted to check that a declaration belongs to the current file, if so move it to the top of completion list.

So we don't yet get the "local" group? That should be done by returning the proper scope from within DeclarationItem::completionProperties. But I thought/hoped that this should be done automatically? See DUChainUtils::completionProperties which does something at the end with the scope. If thats buggy, lets fix it there.

Anyhow, as I said - please split this patch into smaller ones for easier review.


- Milian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115446/#review48824
-----------------------------------------------------------


On Feb. 3, 2014, 12:25 p.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115446/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2014, 12:25 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> *Don't insert arguments of a function into text on execute(..)
> *Append () to functions 
> *Properly (I hope...) handle overloaded functions.
> *Show local items above others.
> 
> P.S: Now, when I've got commit rights, should I still post these patches for review or not?
> 
> 
> Diffs
> -----
> 
>   codecompletion/context.cpp 6c4df1c 
> 
> Diff: https://git.reviewboard.kde.org/r/115446/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

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