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.
> 
> Ivan Shapovalov wrote:
>     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?
> 
> Olivier Jean de Gaalon wrote:
>     fake-local-enum:
>     Don't know how I missed that change there, it is your fault ;). In your given test case I get double items for "foo" and "bar" thanks to that. Could you make sure it's not already in scope before adding it? Should be possible to change the function in question.
>     Could you expound on the "weird things" that make this part not testable?
>     
>     whitespace:
>     Duh, sorry. Remove trailing whitespace, yes.

whitespace:
OK, will do.

fake-local-enum:
Fixed; now specialItemsForArgumentType() returns an empty set if the enumerators it's going to add are in local scope. Existing unit-test modified to follow the improvement, TODO comment in that unit-test cleared. Thanks for the hint.
But: in the provided test-case the duplication is of a different kind; items in "Best Matches" section are essentially copies of items from different section. So in the test-case you'll still observe the duplication, but it isn't caused by this patch.

testing 2 and 3:
I think the problem is in DUContext::createRangeMoving(), which is used to get the switch'd expression to parse its type. In test-case mode it becomes invalid and returns empty text since class PersistentMovingRange itself is not yet completely implemented. Do you know other ways of getting text from a DUChain object?
For now, I've added required unit-tests, marked them with QEXPECT_FAIL and left an explanation in code.

demoting vs hiding:
I'd prefer demoting. Either we'll have overly-complicated rules for hiding, or we'll start to get bug-reports from users with unusual use-cases.
Anyway, the _definitely_matching_ items are highlighted and placed before others, so why bother?

----
BTW, there is a small usability issue. An item that is created by specialItemsForArgumentType() (which in our case represents an enumerator from _different_ scope) has its alternative text set, which in turn makes code in item.cpp to return an empty type prefix.
You may see it in the test-case: as long as the enumeration is non-global, type prefix is empty; when you make the enum global, type appears.
My next patch fixes it, but please evaluate: is it OK, or it'd be better without type prefix displayed?


- Ivan


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


On July 5, 2012, 1:29 p.m., Ivan Shapovalov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105410/
> -----------------------------------------------------------
> 
> (Updated July 5, 2012, 1:29 p.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/codecompletion/item.cpp affd4e6 
>   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/0cb128a4/attachment.html>