Review Request 117878: Implements a luajit backend for Cantor

Mon May 5 16:32:37 UTC 2014

> On May 4, 2014, 3:56 p.m., Alexander Rieder wrote:
> > 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.

Thanks for the reviews, I think your suggestion were very precise and helpful.

> On May 4, 2014, 3:56 p.m., Alexander Rieder wrote:
> > src/backends/lua/luasession.cpp, line 69
> > <https://git.reviewboard.kde.org/r/117878/diff/3/?file=271258#file271258line69>
> >
> >     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.

Thanks! I've removed it now.

I've tested the Python backend now with a 'while True: pass', and it seems that it is also synchronous, so maybe there is no need for the list there too.

> On May 4, 2014, 3:56 p.m., Alexander Rieder wrote:
> > src/backends/lua/luasession.cpp, line 45
> > <https://git.reviewboard.kde.org/r/117878/diff/3/?file=271258#file271258line45>
> >
> >     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);

Done.

> On May 4, 2014, 3:56 p.m., Alexander Rieder wrote:
> > src/backends/lua/luaexpression.cpp, line 55
> > <https://git.reviewboard.kde.org/r/117878/diff/3/?file=271250#file271250line55>
> >
> >     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) 
> >

Thanks, I did not saw that before.

> On May 4, 2014, 3:56 p.m., Alexander Rieder wrote:
> > src/backends/lua/luaexpression.cpp, line 47
> > <https://git.reviewboard.kde.org/r/117878/diff/3/?file=271250#file271250line47>
> >
> >     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

Done.

> On May 4, 2014, 3:56 p.m., Alexander Rieder wrote:
> > src/backends/lua/luaexpression.h, line 42
> > <https://git.reviewboard.kde.org/r/117878/diff/3/?file=271249#file271249line42>
> >
> >     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?
> >

Indeed. Fixed (joined execute() and eval(), since execute is small and only called from eval() ). 

> On May 4, 2014, 3:56 p.m., Alexander Rieder wrote:
> > src/backends/lua/luacompletionobject.cpp, line 50
> > <https://git.reviewboard.kde.org/r/117878/diff/3/?file=271248#file271248line50>
> >
> >     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)
> >

Perfect. Easier to read and more 'robust'.

> On May 4, 2014, 3:56 p.m., Alexander Rieder wrote:
> > src/backends/lua/luabackend.cpp, line 44
> > <https://git.reviewboard.kde.org/r/117878/diff/3/?file=271243#file271243line44>
> >
> >     just for the sake of consistency across all backends, I think this should be lowercase

Done.

- Lucas

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

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/20140505/4a12a321/attachment-0001.html>