Review Request 117878: Implements a luajit backend for Cantor

[Alexander Rieder]

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


Hi,
I've had a closer look at your code and it already looks pretty good, and from my limited knowledge of lua it seems to work well. 
There still some minor things I've noticed going through the code, please have a look and tell me what you think.


src/backends/lua/luabackend.cpp
<https://git.reviewboard.kde.org/r/117878/#comment39902>

    just for the sake of consistency across all backends, I think this should be lowercase



src/backends/lua/luacompletionobject.cpp
<https://git.reviewboard.kde.org/r/117878/#comment39908>

    from what i can see, you are stripping whitespaces from the command with that loop. Is QString::trimmed() what you are looking for? (easier to read and might also catch "weird" whitespaces)
    



src/backends/lua/luaexpression.h
<https://git.reviewboard.kde.org/r/117878/#comment39905>

    You have methods named evaluate(), execute() and eval() in the same class. Could you come up with more descriptive names for the latter two, to make the difference of what they do more clear to the casual reader?
    



src/backends/lua/luaexpression.cpp
<https://git.reviewboard.kde.org/r/117878/#comment39906>

    It might be a personal style choice, but I don't like declaring two different variable types in one line like this, IMHO it makes it easy to overlook the second declaration. Please put a line break



src/backends/lua/luaexpression.cpp
<https://git.reviewboard.kde.org/r/117878/#comment39907>

    I doesn't make much visual difference right now, but for errors you shouldn't be using a text result, but instead use Expression::setErrorMessage(), as we'd like to keep errors
    and proper results separated if possible (this is also what gives you the empty line between command and result if you enter an invalid expression) 
    



src/backends/lua/luasession.cpp
<https://git.reviewboard.kde.org/r/117878/#comment39903>

    lease avoid the overlong line. its very inconvenient to read.
    i think it would be more readable if you did somethin like
    const char* printCmd="function print(...) local t={};"  
            "for i = 1, select('#',...) do;"
            "  local a=select(i,...); t[i]=tostring(a);"
            "end;"
            "table.insert(__cantor, table.concat(t,'\t'))"
            "end";
    luahelper_loadstring(m_L, printCmd);



src/backends/lua/luasession.cpp
<https://git.reviewboard.kde.org/r/117878/#comment39904>

    In the Session you have a m_runningExpressions list, which I assume you took from one of the other backends, but you do not 
    seem to be using it properly, i.e. you never add anything to it, but you sometimes clear it, or try to iterate over it.
    It works fine because you implemented the backend in a synchroneous way, i.e. the session does only return control from
    LuaSession::evaluateExpression() once the expression has been fully evaluated, so there never can be more than one "active" expression.
    In backends like maxima or sage the runningExpression list is used, because all the calculation is done in a separate process, so evaluateExpression
    returns immediately and the user may press evaluate again, before the maxima process has had time to finish the calculation of the first expression.
    Therefore we push the new expression on the list, and it will be executed once maxima is done with the old expression.
    As long as you don't want to move the calculation into the background(either via threads or a separate process), I think you should just remove the list. it doesn't do anything anyway.


besides these minor issues, its good work!

best regards,
Alexander

- Alexander Rieder


On May 3, 2014, 12:55 a.m., Lucas Hermann Negri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117878/
> -----------------------------------------------------------
> 
> (Updated May 3, 2014, 12:55 a.m.)
> 
> 
> Review request for KDE Edu, Alexander Rieder and Filipe Saraiva.
> 
> 
> Repository: cantor
> 
> 
> Description
> -------
> 
> Implements a luajit backend for Cantor. Implements highlighting, completion, and other features (variable explorer is missing).
> 
> Is compatible with Lua science/math libraries like numlua[1] and lna[2].
> 
> [1]: https://github.com/carvalho/numlua
> [2]: https://bitbucket.org/lucashnegri/lna
> 
> 
> Diffs
> -----
> 
>   cmake/FindLuaJIT.cmake PRE-CREATION 
>   icons/hi48-app-luabackend.png PRE-CREATION 
>   src/backends/CMakeLists.txt e89d9e6 
>   src/backends/lua/CMakeLists.txt PRE-CREATION 
>   src/backends/lua/luabackend.cpp PRE-CREATION 
>   src/backends/lua/luabackend.desktop PRE-CREATION 
>   src/backends/lua/luabackend.h PRE-CREATION 
>   src/backends/lua/luabackend.kcfg PRE-CREATION 
>   src/backends/lua/luacompletionobject.h PRE-CREATION 
>   src/backends/lua/luacompletionobject.cpp PRE-CREATION 
>   src/backends/lua/luaexpression.h PRE-CREATION 
>   src/backends/lua/luaexpression.cpp PRE-CREATION 
>   src/backends/lua/luaextensions.h PRE-CREATION 
>   src/backends/lua/luaextensions.cpp PRE-CREATION 
>   src/backends/lua/luahelper.h PRE-CREATION 
>   src/backends/lua/luahelper.cpp PRE-CREATION 
>   src/backends/lua/luahighlighter.h PRE-CREATION 
>   src/backends/lua/luahighlighter.cpp PRE-CREATION 
>   src/backends/lua/luasession.h PRE-CREATION 
>   src/backends/lua/luasession.cpp PRE-CREATION 
>   src/backends/lua/settings.kcfgc PRE-CREATION 
>   src/backends/lua/settings.ui PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/117878/diff/
> 
> 
> Testing
> -------
> 
> Basic testing performed manually. Everything seems to work as expected.
> 
> 
> Thanks,
> 
> Lucas Hermann Negri
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20140504/093e8101/attachment.html>