Review Request: Proper auto-completion in switch statements

[Olivier Jean de Gaalon]

> On July 5, 2012, 10:55 p.m., Olivier Jean de Gaalon wrote:
> > Without time for a proper review...
> > Demoting vs hiding: item.cpp is actually (I'm quite sure) wrongly promoting items, otherwise you'd never have to manually demote them to 0. I'm actually thinking you should lose the demote, and if you want you can send another patch to fix item.cpp.
> > 
> > The alternative text should only be blank if the enum is local (which shouldn't happen anymore), otherwise it should be something like "namespace::enumname::foo". Is that not the case?
> > If it's not, specialItemsForArgumentType needs to be fixed to provide proper alternative text for inserted items, or not use alt text at all.
> 
> Ivan Shapovalov wrote:
>     Demoting vs hiding: item.cpp AFAIK doesn't know whether we are completing for a type or instance; it uses only type-id to do its matching (the type is contained in field currentMatchContext).
>     But well, I'll dig in a bit more. Maybe we could pass "type or instance" flag based on the completion context's type.
>     
>     Alt-text: yes, it's the case. Though the issue is a bit different: code in item.cpp forcedly returns an empty type-name prefix if alternative text is set.
>     
>     So, for _local_ enumerations we see following in the completion window:
>     ------------------------
>     someEnumTypeName  |  foo <-- here "foo" and "someEnumTypeName" are generated by Cpp::NormalDeclarationCompletionItem::data()
>     someEnumTypeName  |  bar
>     ------------------------
>     
>     And for non-local enumerations we see following:
>     -----------------------
>       |  someNamespace::foo <-- here "someNamespace::foo" is the alternative text (which, being set, makes type column empty)
>       |  someNamespace::bar
>     -----------------------
>     
>     My fix makes the latter case look like:
>     ------------------------------------------------------
>     someNamespace::someEnumTypeName  |  someNamespace::foo
>     someNamespace::someEnumTypeName  |  someNamespace::bar
>     ------------------------------------------------------
>     
>     Which one is better? That fix makes "someNamespace::" to be repeated twice per line, is it OK?

There is a rather serious problem here, unfortunately. You cannot aquire the ForegroundLock when the duchain is locked, or you get deadlocks. That means that you cannot call doCaseCompletion in the constructor at all, as the DUChain is guaranteed to be read-locked throughout. The only way to change this would be to unlock before calling  doCaseCompletion() and getParentContext() (since getParentContext is where the CaseAccess context is constructed) then relock immediately after and check m_duContext again. This is a bit ugly/verbose... if you can, try to get the case expression type at another point.
(/me mutters something about the threading model and welcomes you to the dark side of KDevelop)
Once you fix this, make sure to replace the ENSURE_CHAIN_NOT_LOCKED before aquiring the ForegroundLock... and add a comment explaining that it should never be removed.

As I was saying before I noticed...
-------------------------------------
I'm not in favor of /adding/ code to fix subsets of problems caused by other code, when the other code can be changed (albeit less simply). I would prefer you keep your other fixes, and simply drop that one from this patch. Perhaps in another review we can hash out the best fix for that.

Your current fix for the alt text issue breaks signal/slot completion items which also use alt text. I think the correct fix would be to not set or use the alt text on the specialItemsForArgumentType. That would provide adequate information and look ok.. no?

Also, add a comment explaining the "if (prefix.isEmpty) return" for posterity.


- Olivier Jean de


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


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/20120706/825e5199/attachment.html>