Review Request: Proper auto-completion in switch statements

Olivier Jean de Gaalon olivier.jg at gmail.com
Thu Jul 5 11:28:09 UTC 2012



> 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?

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.


- Olivier Jean de


-----------------------------------------------------------
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/20d6c4ea/attachment.html>


More information about the KDevelop-devel mailing list