Review Request 117458: Support for function expressions

Thu Apr 10 15:12:25 UTC 2014

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117458/#review55388
-----------------------------------------------------------

Looks nice overall, but you're doing three things at once in this patch: add support for function expressions, add support for binary operators, and move code around. It would have been far easier to review if that were three separate patches (or rather, the latter two went in without review and just the first one was reviewed) ;)
It would be cool if you could try to avoid that in the future.

Other than that it seems fine except for the comments below. Support for var x = (function(){return 3;})() is certainly fancy! ;)

duchain/declarationbuilder.cpp
<https://git.reviewboard.kde.org/r/117458/#comment38564>

    Did you try this in some real-world JS code? Judging from what it is like in Python, something like this might have so many false positives that it's likely to do more harm than good ...
    Instead, I would merge the old and new return type into an unsure type.

duchain/declarationbuilder.cpp
<https://git.reviewboard.kde.org/r/117458/#comment38566>

    Please separate code cleanup commits (moving code around) from changes. It makes patches hard to review.

duchain/expressionvisitor.cpp
<https://git.reviewboard.kde.org/r/117458/#comment38567>

    nullptr? or just no argument?

duchain/expressionvisitor.cpp
<https://git.reviewboard.kde.org/r/117458/#comment38568>

    Not the most elegant way to check this ;)
    How about static_cast<int>(node->value) == node->value (if that works) or something like that?
    QString::number() will allocate memory and is thus infinitely slower than most other ways of doing it.

duchain/expressionvisitor.cpp
<https://git.reviewboard.kde.org/r/117458/#comment38570>

    Not to be fixed for this patch, but that won't work; you will need to represent a string differently, given that it also has e.g. a "length" attribute etc.
    In Python I have a file which contains classes for the built-in types and then those are instantiated, that works quite well. I think you can do something similar in JS.

duchain/helper.h
<https://git.reviewboard.kde.org/r/117458/#comment38572>

    Do you really need to treat void as useless? void is a valid type, it might be the correct guess. And the information that e.g. a function returns void is useful (while "it returns mixed" is not).

- Sven Brauch

On April 10, 2014, 12:29 p.m., Denis Steckelmacher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117458/
> -----------------------------------------------------------
> 
> (Updated April 10, 2014, 12:29 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-qmljs
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   duchain/CMakeLists.txt 9fab69c 
>   duchain/contextbuilder.cpp d38da2d 
>   duchain/declarationbuilder.h 001c3b2 
>   duchain/declarationbuilder.cpp 98da341 
>   duchain/expressionvisitor.h b4f0851 
>   duchain/expressionvisitor.cpp 766435d 
>   duchain/helper.h 23a6d0e 
>   duchain/helper.cpp 3ffdd56 
>   duchain/parsesession.h 0eb2762 
>   duchain/parsesession.cpp dbc4d90 
>   duchain/tests/testdeclarations.cpp 668efaa 
>   tests/files/helloworld.js ff40f1c 
> 
> Diff: https://git.reviewboard.kde.org/r/117458/diff/
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> Denis Steckelmacher
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140410/31d5ff28/attachment.html>