Review Request: Proper auto-completion in switch statements

[Ivan Shapovalov]

> On July 4, 2012, 11:40 p.m., Olivier Jean de Gaalon wrote:
> > I don't have time to do a proper review right now, but with a quick look-over... Once you've gone through the trouble of finding out what items are non-const, why bother demoting them? Is there a reason they should be included in the possible completions at all?
> 
> Ivan Shapovalov wrote:
>     Of course, there is a reason.
>     One could possibly do a case branch against some constexpr function, or a class/namespace member (again, constant).
>     There is no possibility (at least with current parsing quality) to get an exhaustive list of completions in all scopes.
> 
> Olivier Jean de Gaalon wrote:
>     Obviously you need to keep types and namespaces, but if you run into a non-const function it's an easy trim. Not that important as it's not a current feature anyhow, and I'm not sure how well our C++ knows about constexpr anyhow...
> 
> Olivier Jean de Gaalon wrote:
>     More properly now...
>     1. No whitespace changes, please
>     2. You should be able to test item 2 with CompletionItemTester::itemData() if you pass CodeCompletionModel::MatchQuality as the "role"
>     
>     I don't really like demoting items instead of simply not promoting/including them, but it's good enough.
>     It's not your fault, but I wouldn't mind if it didn't insert the fake-local-enum completion items anymore since the global ones are now already considered to be the best match.
>     
>     With points 1 and 2 dealt with it's good enough, I think, unless you feel like digging a bit more.

What do you mean by "not inserting the fake-local-enum completion items"? If you're talking about specialItemsForArgumentType(), then it's clearly needed - it was the point of the whole patch to call that function not only for BinaryOpFunctionCallAccess, but also for CaseAccess..

And about whitespace - do you need me to rtrim the patch, or what?


- Ivan


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


On July 4, 2012, 9:49 a.m., Ivan Shapovalov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105410/
> -----------------------------------------------------------
> 
> (Updated July 4, 2012, 9:49 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Improve code completion within switch statements.
> 
> 1) Resolve type of the switch'd expression correctly
>  - in switchExpressionType(), replaced evaluateType() with evaluateExpression()
> 
> 2) Correctly complete code for enumerations (e. g. do not mark type declarations as matching items)
>  - this required adding integral constant check in CodeCompletionContext::standardAccessCompletionItems()
> 
> 3) Add completion items for enumerations declared in different scopes
>  - this required adding another condition branch in the end of CodeCompletionContext::standardAccessCompletionItems(),
> which in turn required proper setting of m_expressionResult in CaseAccess contexts,
> which required moving switchExpressionType() to doCaseCompletion() to avoid code duplication.
> 
> 
> Diffs
> -----
> 
>   languages/cpp/codecompletion/context.h a5fdea7 
>   languages/cpp/codecompletion/context.cpp 33dcad1 
>   languages/cpp/tests/test_cppcodecompletion.h 20a70cb 
>   languages/cpp/tests/test_cppcodecompletion.cpp ec82d2d 
> 
> Diff: http://git.reviewboard.kde.org/r/105410/diff/
> 
> 
> Testing
> -------
> 
> there is a unit-test for change (1),
> I don't know how to test (2) and
> (3) cannot be tested due to some weird things with CompletionItemTester though it can be tested manually.
> 
> 
> Thanks,
> 
> Ivan Shapovalov
> 
>

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