Review Request: Correctly set completion properties for enumerators

[Ivan Shapovalov]

> On July 19, 2012, 11:07 a.m., Olivier Jean de Gaalon wrote:
> > See kdevelop/cpp/codecompletion/item.cpp:315, it looks like it's going to override whatever you do here. Thankfully that commit also gives us some context:
> > "
> > - Implement a nice new feature: When completing for a function argument of enumeration type, show the fitting enumerations directly, and if they aren't in visible scope, show them with the necessary scope added.
> > - Make the public/private grouping work for enumerators.
> > "
> > So... to recap:
> > DUChainUtils::completionProperties probably used to do what you're making it do now.
> > The CPP plugin's NormalDeclarationCompletionItem has some funky code specifically to override what DUChainUtils /used/ to do, but doesn't do anymore (but that you're making it do again).
> > 
> > So, I don't know the reasoning for the way it is now, and I agree with your reasoning for how it should be.
> > If you're going to fix it, just make sure:
> > 1. You find out what NormalDeclarationCompletionItem is trying to accomplish
> > 2. You don't break the feature you've been working on all this time (showing matching out-of-scope enums)
> > 3. You make sure that public/private grouping still works for enumerators
> > 4. All tests pass
> > 
> > You might want to trawl through git log and history to find out WTF is going on.

Well, there's even more bigger mess, and that "commit in queue" actually fixes what you're talking about.

See:
in NormalDeclarationCompletionItem::completionProperties() there is a "if (dec->type<...>())" _under_ "case AbstractType::TypeIntegral:".
These conditions are completely contradictory - at least, for C++; EnumeratorType (as well as EnumerationType) has its type set to AbstractType::TypeEnumerator and AbstractType::TypeEnumeration respectively.
So we've got a dead code. (I've tried to put there a Q_ASSERT(false), which had never executed.)

Moreover, current implementation of kdevplatform's DUChainUtils::completionProperties() sets "CodeCompletionModel::Enum" on enumerators - but that dead code is not only dead, it's wrong because it tries to set "CodeCompletionModel::Enum" once again.

And one more thing: inheriting properties from owner should bring not only "private/public", but also "local/global" to enumerators. Currently (as of git master), enumerators have neither Local nor Global flag set on them. So that code is dead indeed.
That can be easily seen if you try to write "int a = " somewhere in KDevelop's code, and then invoke completion on it. If you scroll very down (but before Kate's text completion), you'll see an _unnamed_ group under which all available enumerators reside.

Finally, I'm going to 1) fix kdevplatform's DUChainUtils::completionProperties(), 2) remove contradicting conditions from NormalDeclarationCompletionItem::completionProperties() and 3) remove all bit-manipulation from the same function.

All tests pass - and actually I'm working with all these changes for 2 days.


- Ivan


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


On July 19, 2012, 7:42 a.m., Ivan Shapovalov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105616/
> -----------------------------------------------------------
> 
> (Updated July 19, 2012, 7:42 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> EnumeratorType is rather a variable (compile-time integral constant) than an enum type-name, so mark it accordingly.
> QUESTION: although this is valid for C/C++, are there any places where an EnumeratorType would represent something different?
> 
> // this alone won't fix any bugs, there is a dependent change in queue
> 
> 
> Diffs
> -----
> 
>   language/duchain/duchainutils.cpp 1879540 
> 
> Diff: http://git.reviewboard.kde.org/r/105616/diff/
> 
> 
> Testing
> -------
> 
> Existing unit-tests pass.
> 
> 
> Thanks,
> 
> Ivan Shapovalov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120719/4f0b519f/attachment.html>