<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105410/">http://git.reviewboard.kde.org/r/105410/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 4th, 2012, 11:40 p.m., <b>Olivier Jean de Gaalon</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On July 5th, 2012, 4:15 a.m., <b>Ivan Shapovalov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On July 5th, 2012, 10:01 a.m., <b>Olivier Jean de Gaalon</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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...</pre>
</blockquote>
<p>On July 5th, 2012, 10:37 a.m., <b>Olivier Jean de Gaalon</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On July 5th, 2012, 10:54 a.m., <b>Ivan Shapovalov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On July 5th, 2012, 11:28 a.m., <b>Olivier Jean de Gaalon</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
<br />
<p>- Ivan</p>
<br />
<p>On July 5th, 2012, 1:29 p.m., Ivan Shapovalov wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Ivan Shapovalov.</div>
<p style="color: grey;"><i>Updated July 5, 2012, 1:29 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>languages/cpp/codecompletion/context.h <span style="color: grey">(a5fdea7)</span></li>
<li>languages/cpp/codecompletion/context.cpp <span style="color: grey">(33dcad1)</span></li>
<li>languages/cpp/codecompletion/item.cpp <span style="color: grey">(affd4e6)</span></li>
<li>languages/cpp/tests/test_cppcodecompletion.h <span style="color: grey">(20a70cb)</span></li>
<li>languages/cpp/tests/test_cppcodecompletion.cpp <span style="color: grey">(ec82d2d)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105410/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>