<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/117878/">https://git.reviewboard.kde.org/r/117878/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 4th, 2014, 3:56 p.m. UTC, <b>Alexander Rieder</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;">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.</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;">Thanks for the reviews, I think your suggestion were very precise and helpful.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 4th, 2014, 3:56 p.m. UTC, <b>Alexander Rieder</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/117878/diff/3/?file=271243#file271243line44" style="color: black; font-weight: bold; text-decoration: underline;">src/backends/lua/luabackend.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">44</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="s">"Lua"</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">just for the sake of consistency across all backends, I think this should be lowercase</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Done.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 4th, 2014, 3:56 p.m. UTC, <b>Alexander Rieder</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/117878/diff/3/?file=271248#file271248line50" style="color: black; font-weight: bold; text-decoration: underline;">src/backends/lua/luacompletionobject.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">50</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span> <span class="k">while</span><span class="p">(</span><span class="n">idx</span> <span class="o"><</span> <span class="n">name</span><span class="p">.</span><span class="n">length</span><span class="p">()</span> <span class="o">&&</span> <span class="n">name</span><span class="p">[</span><span class="n">idx</span><span class="p">]</span> <span class="o">==</span> <span class="sc">' '</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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)
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Perfect. Easier to read and more 'robust'.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 4th, 2014, 3:56 p.m. UTC, <b>Alexander Rieder</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/117878/diff/3/?file=271249#file271249line42" style="color: black; font-weight: bold; text-decoration: underline;">src/backends/lua/luaexpression.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">42</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="nf">execute</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Indeed. Fixed (joined execute() and eval(), since execute is small and only called from eval() ). </pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 4th, 2014, 3:56 p.m. UTC, <b>Alexander Rieder</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/117878/diff/3/?file=271250#file271250line47" style="color: black; font-weight: bold; text-decoration: underline;">src/backends/lua/luaexpression.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">47</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">ret</span><span class="p">;</span> <span class="n">Cantor</span><span class="o">::</span><span class="n">Expression</span><span class="o">::</span><span class="n">Status</span> <span class="n">status</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Done.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 4th, 2014, 3:56 p.m. UTC, <b>Alexander Rieder</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/117878/diff/3/?file=271250#file271250line55" style="color: black; font-weight: bold; text-decoration: underline;">src/backends/lua/luaexpression.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">55</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nf">setResult</span><span class="p">(</span><span class="k">new</span> <span class="n">Cantor</span><span class="o">::</span><span class="n">TextResult</span><span class="p">(</span><span class="n">ret</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<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 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)
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks, I did not saw that before.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 4th, 2014, 3:56 p.m. UTC, <b>Alexander Rieder</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/117878/diff/3/?file=271258#file271258line45" style="color: black; font-weight: bold; text-decoration: underline;">src/backends/lua/luasession.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">45</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">errors</span> <span class="o"><<</span> <span class="n">luahelper_dostring</span><span class="p">(</span><span class="n">m_L</span><span class="p">,</span> <span class="s">"function print(...) local t={}; for i = 1, select('#',...) do local a=select(i,...); t[i]=tostring(a); end; table.insert(__cantor, table.concat(t,'</span><span class="se">\t</span><span class="s">')) end"</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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);</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Done.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 4th, 2014, 3:56 p.m. UTC, <b>Alexander Rieder</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/117878/diff/3/?file=271258#file271258line69" style="color: black; font-weight: bold; text-decoration: underline;">src/backends/lua/luasession.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">69</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">foreach</span><span class="p">(</span><span class="n">Cantor</span><span class="o">::</span><span class="n">Expression</span><span class="o">*</span> <span class="n">e</span><span class="p">,</span> <span class="n">m_runningExpressions</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Lucas</p>
<br />
<p>On May 3rd, 2014, 12:55 a.m. UTC, Lucas Hermann Negri 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 KDE Edu, Alexander Rieder and Filipe Saraiva.</div>
<div>By Lucas Hermann Negri.</div>
<p style="color: grey;"><i>Updated May 3, 2014, 12:55 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
cantor
</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;">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</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;">Basic testing performed manually. Everything seems to work as expected.</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>cmake/FindLuaJIT.cmake <span style="color: grey">(PRE-CREATION)</span></li>
<li>icons/hi48-app-luabackend.png <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/CMakeLists.txt <span style="color: grey">(e89d9e6)</span></li>
<li>src/backends/lua/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luabackend.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luabackend.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luabackend.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luabackend.kcfg <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luacompletionobject.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luacompletionobject.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luaexpression.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luaexpression.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luaextensions.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luaextensions.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luahelper.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luahelper.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luahighlighter.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luahighlighter.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luasession.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/luasession.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/settings.kcfgc <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/backends/lua/settings.ui <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117878/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>