<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/4535/">https://reviewboard.asterisk.org/r/4535/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 26th, 2015, 11:57 p.m. CET, <b>Diederik de Groot</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/4535/diff/1/?file=72977#file72977line1236" style="color: black; font-weight: bold; text-decoration: underline;">/branches/13/include/asterisk/strings.h</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 force_inline char *attribute_pure ast_str_to_upper(char *str)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1236</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">struct</span> <span class="n">ao2_container</span> <span class="o">*</span><span class="n">ast_str_container_alloc_options</span><span class="p">(</span><span class="k">enum</span> <span class="n">ao2_container_opts</span> <span class="n">opts</span><span class="p">,</span> <span class="kt">int</span> <span class="n">buckets</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1236</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1"><span class="hl">//</span>struct ao2_container *ast_str_container_alloc_options(enum ao2_container_opts opts, int buckets);</span></pre></td>
</tr>
</tbody>
<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">1237</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">struct</span> <span class="n">ao2_container</span> <span class="o">*</span><span class="n">ast_str_container_alloc_options</span><span class="p">(</span><span class="k">enum</span> <span class="n">ao2_alloc_opts</span> <span class="n">opts</span><span class="p">,</span> <span class="kt">int</span> <span class="n">buckets</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;">Not 100% sure if this substition is correct. Please verify.</pre>
</blockquote>
<p>On March 28th, 2015, 3:43 p.m. CET, <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;">Well, it's "correct", but odd.
Most container allocation routines take in two parameters - the container options, and the ao2 allocation options. This is taking in the ao2_container_opts, but treating it as if it were the ao2 allocation options. Two factors make me think the code change you have here is correct, and that we should not bother exposing ao2_container_opts:
(1) Most container options deal with how things are stored internally and traversal mechanisms. For a simple bucket of strings, those options aren't generally useful.
(2) The usage of the function involves attempting to create a container of strings without a lock (AO2_ALLOC_OPT_LOCK_NOLOCK), which is an ao2 allocation option.
So I think this is correct - the first parameter should be of type enum ao2_alloc_opts.</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;">I was a little worried about doing something wrong here. Casting enums to int to make them pass is always a sign of problem ahead. Good that clang is a little more vigilant about these.</pre>
<br />
<p>- Diederik</p>
<br />
<p>On March 28th, 2015, 4:27 p.m. CET, Diederik de Groot 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 Diederik de Groot.</div>
<p style="color: grey;"><i>Updated March 28, 2015, 4:27 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-24917">ASTERISK-24917</a>
</div>
<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;">clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs
clang compiler warning:-Wenum-conversion
Changes:
/branches/13/channels/chan_pjsip.c:923
Wrong enum being used
/branches/13/channels/chan_sip.c:19102
Incorrect enum
/branches/13/channels/chan_sip.c:19107
Incorrect enum returned
/branches/13/include/asterisk/strings.h:1236
Replaced enum ao2_container_opts opts -> enum ao2_alloc_opts opts
Not 100% sure if this substition is correct. Please verify.
/branches/13/main/strings.c:173
Replaced enum ao2_container_opts opts -> enum ao2_alloc_opts opts
Not 100% sure if this substition is correct. Please verify.
/branches/13/res/res_stasis.c:1803
Incorrect enum for return value
</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>/branches/13/res/res_stasis.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/main/strings.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/include/asterisk/strings.h <span style="color: grey">(433444)</span></li>
<li>/branches/13/channels/chan_sip.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/channels/chan_pjsip.c <span style="color: grey">(433444)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4535/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>