<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://reviewboard.asterisk.org/r/3698/">https://reviewboard.asterisk.org/r/3698/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 2nd, 2014, 10:14 a.m. CDT, <b>Tilghman Lesher</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://reviewboard.asterisk.org/r/3698/diff/1/?file=61842#file61842line3390" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/res/ael/pval.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int gen_prios(struct ael_extension *exten, char *label, pval *statement, struct ael_extension *mother_exten, struct ast_context *this_context )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3390</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">switch_set</span><span class="o">-></span><span class="n">app</span> <span class="o">=</span> <span class="n">strdup</span><span class="p">(</span><span class="s">"Set"</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">3386</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">switch_set</span><span class="o">-></span><span class="n">app</span> <span class="o">=</span> <span class="n">strdup</span><span class="p">(</span><span class="s">"Set"</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;">There are implications to how the language compilation works that are satisfied by using MSet. Unless you're going to revisit how AEL works, I'd suggest keeping this as always running MSet in the future.</pre>
</blockquote>
<p>On July 3rd, 2014, 6:36 a.m. CDT, <b>Matt Jordan</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;">Can you explain this a bit more? I'm not sure how MSet is required here. In each of the cases here, it appears as if a single variable is being set, which - from a cursory look - should be able to be satisfied using Set instead of MSet. I could easily be missing something here however.</pre>
</blockquote>
<p>On July 3rd, 2014, 9:07 a.m. CDT, <b>Tilghman Lesher</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;">Consider this line from configs/extensions.conf.sample:
CONSOLE-AEL="Console/dsp";
This get translated to:
MSet(CONSOLE-AEL=$["Console/dsp"])
The expression parser will evaluate a string as itself, and MSet will remove the quotes. If the quotes are not removed, consider what would happen when you tried to Dial that variable:
Dial("Console/dsp")
The Dial app will parse that and attempt to find a technology called '"Console', which will fail.
Now, let's suppose that you modified the AEL app to remove the quotes. Sounds plausible, right? Except that you now need to consider what happens if the first character in a string is a digit:
MSet(foo=$[1234-office])
This would get a parse error, and the evaluation would come back to the empty string, almost certainly not what the person writing AEL intended. So fine, let's remove the expression parser, too:
time_left = ${somevar} - ${someothervar}
Now, this (other) expression will fail, whereas with the expression parser, it would perform as needed. Even worse, consider that you just won't know at compile time (without a LOT of work) whether a particular argument (due to variable interpolation) might be interpreted as an arithmetic expression or simply as a string. Bottom line, unless you're going to go through the AEL code and figure out another way of parsing AEL expressions, all of which have to happen at compile time, with loosely typed variables, you're best to leave in MSet.</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 for the explanation. When looking at this I was considering whether or not what was passed was a multiple variable behavior, and hadn't considered the quoting aspect.
Given that, it sounds like the right thing to do right now is:
(1) Remove the deprecation warning from MSet. If we use something internally, we can't deprecate it.
(2) Make AEL use MSet by default</pre>
<br />
<p>- Matt</p>
<br />
<p>On July 1st, 2014, 7:54 p.m. CDT, Matt Jordan wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.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 Asterisk Developers.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated July 1, 2014, 7:54 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</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;">Per the discussion on the asterisk-dev mailing list [1], this patch removes the following:
* cdr_sqlite
* chan_gtalk
* chan_jingle
* chan_h323
* res_jabber
* app_saycountpl
* app_readfile
* app_dahdibarge
It removes the following applications/functions:
* WaitMusicOnHold
* SetMusicOnHold
* SIPCHANINFO
And it removes the colon delimiter from the SIPPEER function.
It also removes all compatibility options that were configurable from asterisk.conf, as these all applied to compatibility with Asterisk 1.4 systems.
Corey pointed out a number of other deprecated applications/functions, and those should get removed as well - but I wanted to get round #1 up and going since the channel drivers in particular are a bit odious with the media rework going on.
[1] http://lists.digium.com/pipermail/asterisk-dev/2014-June/068363.html</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;">Asterisk compiles without the various modules and loads correctly.</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>/trunk/utils/conf2ael.c <span style="color: grey">(417729)</span></li>
<li>/trunk/utils/ael_main.c <span style="color: grey">(417729)</span></li>
<li>/trunk/res/res_musiconhold.c <span style="color: grey">(417729)</span></li>
<li>/trunk/res/res_jabber.c <span style="color: grey">(417729)</span></li>
<li>/trunk/res/res_agi.c <span style="color: grey">(417729)</span></li>
<li>/trunk/res/ael/pval.c <span style="color: grey">(417729)</span></li>
<li>/trunk/pbx/pbx_realtime.c <span style="color: grey">(417729)</span></li>
<li>/trunk/main/pbx.c <span style="color: grey">(417729)</span></li>
<li>/trunk/main/asterisk.c <span style="color: grey">(417729)</span></li>
<li>/trunk/include/asterisk/options.h <span style="color: grey">(417729)</span></li>
<li>/trunk/configs/jingle.conf.sample <span style="color: grey">(417729)</span></li>
<li>/trunk/configs/jabber.conf.sample <span style="color: grey">(417729)</span></li>
<li>/trunk/configs/gtalk.conf.sample <span style="color: grey">(417729)</span></li>
<li>/trunk/configs/asterisk.conf.sample <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/noexport.map <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/compat_h323.cxx <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/compat_h323.h <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/cisco-h225.cxx <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/cisco-h225.h <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/cisco-h225.asn <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/chan_h323.h <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/caps_h323.cxx <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/caps_h323.h <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/ast_ptlib.h <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/ast_h323.cxx <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/ast_h323.h <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/TODO <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/README <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/Makefile.in <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/INSTALL.openh323 <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/h323/ChangeLog <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/chan_sip.c <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/chan_jingle.c <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/chan_h323.c <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/chan_gtalk.c <span style="color: grey">(417729)</span></li>
<li>/trunk/channels/Makefile <span style="color: grey">(417729)</span></li>
<li>/trunk/apps/app_readfile.c <span style="color: grey">(417729)</span></li>
<li>/trunk/apps/app_dahdibarge.c <span style="color: grey">(417729)</span></li>
<li>/trunk/addons/app_saycountpl.c <span style="color: grey">(417729)</span></li>
<li>/trunk/addons/Makefile <span style="color: grey">(417729)</span></li>
<li>/trunk/UPGRADE.txt <span style="color: grey">(417729)</span></li>
<li>/trunk/CHANGES <span style="color: grey">(417729)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3698/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>