<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="https://git.reviewboard.kde.org/r/117458/">https://git.reviewboard.kde.org/r/117458/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 22nd, 2014, 9:50 p.m. UTC, <b>Milian Wolff</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;">looks pretty good, I'd say we can clean it up and commit it to test it out?
some notes on your commit message though:
"ExpressionVisitor stops visiting when it encounters a type (no need to traverse the AST down to its bottom if the top-most expression is enough to determine a type). This gives it a O(1) time complexity." - That is not O(1), as it still depends on the depth of the type it first encounters (which could be far down, no?). Just remove that Big-O stuff ;-)
thanks</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;">I agree, not O(1), e.g. for stacked binary operators it's still O(n).
Sorry Milian, but I think this patch was split up and submitted in separate parts already. Denis, can you make sure you close it (and other reviews which you might no longer intend to submit) as discarded to avoid confusion? Thanks!</pre>
<br />
<p>- Sven</p>
<br />
<p>On April 11th, 2014, 2:38 p.m. UTC, Denis Steckelmacher wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Denis Steckelmacher.</div>
<p style="color: grey;"><i>Updated April 11, 2014, 2:38 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdev-qmljs
</div>
<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;">This patch adds support for function expressions to the QML/JS plugin. This support required the following changes:
* ExpressionVisitor stops visiting when it encounters a type (no need to traverse the AST down to its bottom if the top-most expression is enough to determine a type). This gives it a O(1) time complexity.
* When DeclarationBuilder wants to know the type of an expression, it first visits this expression (to build any needed declaration), then only starts an ExpressionVisitor. This way, ExpressionVisitor knows all the declarations that may occur in the expression and is able to return its complete type. Moreover, if DeclarationBuilder has called getType (and has visited an expression), then will not enter the node it is visiting (as it already did so). This way, DeclarationBuilder keeps an O(n) time complexity.
* These two changes are illustrated by the support of "var f = function() { return true; }" statements.
* This patch also adds support for binary operators ("a == b" is always a boolean, "a << 2" is always an int, etc). This fixes a bug where the type of "(a == 5)" was wrongly guessed as "int"
The support for function expressions is split between DeclarationBuilder and ExpressionVisitor. DeclarationBuilder, when it encounters a function expression, creates a declaration for it, giving it a name built from the hexadecimal representation of its FunctionExpression* node pointer. When DeclarationBuilder sees a return statement, it sets the type of the enclosing function (whether it is a function declaration or a function expression has no importance here). When ExpressionVisitor is used to find the type of an expression, and this expression is a function expression, then ExpressionVisitor will build the hexadecimal name of the function and use getDeclaration to find its type.</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;">All the unit tests of the QML/JS plugin pass with this patch. Three tests have been added:
* The test for type inference is not expected to fail anymore, as the plugin is able to infer simple types
* New test for "(a == b)" expressions
* New test for "function() {}" expressions
Manual testing shows that syntax highlighting is correct, and also the "outline" view. Building the anonymous declarations by passing them an empty range (RangeInRevision()) removes the anonymous function from the outline view and disables any highlighting related to this declaration in the source view. The function can still have a name which can be freely chosen by the plugin.</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>duchain/CMakeLists.txt <span style="color: grey">(9fab69c)</span></li>
<li>duchain/contextbuilder.h <span style="color: grey">(d53b2fe)</span></li>
<li>duchain/contextbuilder.cpp <span style="color: grey">(d38da2d)</span></li>
<li>duchain/declarationbuilder.h <span style="color: grey">(001c3b2)</span></li>
<li>duchain/declarationbuilder.cpp <span style="color: grey">(98da341)</span></li>
<li>duchain/expressionvisitor.h <span style="color: grey">(b4f0851)</span></li>
<li>duchain/expressionvisitor.cpp <span style="color: grey">(766435d)</span></li>
<li>duchain/helper.h <span style="color: grey">(23a6d0e)</span></li>
<li>duchain/helper.cpp <span style="color: grey">(3ffdd56)</span></li>
<li>duchain/parsesession.h <span style="color: grey">(0eb2762)</span></li>
<li>duchain/parsesession.cpp <span style="color: grey">(dbc4d90)</span></li>
<li>duchain/tests/testdeclarations.cpp <span style="color: grey">(668efaa)</span></li>
<li>tests/files/helloworld.js <span style="color: grey">(ff40f1c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117458/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>